tensorflow
refactored configs, added meshes
This PR eventually hopes to add mesh versions of modelnet40. It follows discussions with @ataiya regarding how to manage different versions of the same conceptual dataset despite different data sources and representations. It is not yet complete - but I feel a code example will help further discussion on direction.
Summary: I've refactored ModelNet40 builder class to delegate required implementations to BuilderConfigs. This leads to slightly unexpected code structure for people familiar with TFDS, but does allow for all data associated with modelnet40 to be under the same name scope.
General notes:
- Getting "no examples were yielded" error that looks similar to this, though that's with tfds-nightly and tf-nightly when generating both train and test sets. Test set generates fine if you remove training
SplitGenerator. I'll look into this - but it shouldn't interfere with discussions on structure. - Haven't done tests. Happy to do so, just want to get a better idea of file/class structure first.
- ~~The google internal checksums dir generates an error when importing (I've commented it out)~~
- Visualizing the triangular mesh requires trimesh -
pip install trimesh networkx pyglet.
you might want to do a pull from master, just pushed several google←→github fixes.
@ataiya done. I don't want to mark this as 'ready for review' because it still needs some cleaning up (e.g. tests) but can I get a quick "this looks on the right track" or "this is horrible, structure things as xyz" when you get a chance? Also any preferences you have for file structure. I'm not sure about you, but I'm no fan of monolithic __init__.pys.
Andrea seems to be the best person to review your PR. Thanks Dominic!
Hi Dominic, let me look at this early next week. I did a large amount of global changes that are just about to go live, but need to spend time on ECCV reviews now.
Agreed about the monolithic init.py :) I wanted to have modelnet40.py but I had many other things to keep me busy.
Just flagging one potential issue: ModelNet40 makes sense as a class name (that's how the original authors spell it), but tfds will convert this to model_net40 for use in tfds.load etc. The inconsistency between this and filenames modelnet40.py etc. may come as a surprise to some.
Hey Dominic,
Yes, I have already raised this with TFDS folks, and they already have a change that allows CamelCase to also work. Internally, it's all re-mapped to the model_net40 as that's the class string key python uses.
On Thu, May 7, 2020 at 9:43 PM Dominic Jack [email protected] wrote:
Just flagging one potential issue: ModelNet40 makes sense as a class name (that's how the original authors spell it), but tfds will convert this to model_net40 for use in tfds.load etc. The inconsistency between this and filenames modelnet40.py etc. may come as a surprise to some.
— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/tensorflow/graphics/pull/286#issuecomment-625582888, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCF4PJJ6TKEIOPO5YAW4BLRQNPVTANCNFSM4MYFWHYA .
It seems are are going ahead with BuilderConfig for ShapeNet as well. (we are following TFDS's way of doing things, which is not ideal, but it is standard for now)
@jackd do you want to go ahead with this?
@taiya sorry for the long hiatus, tying up some loose ends. A lot of this is just moving things around to mimic the code structure that was there before, albeit moved around a bit.
AFAIK the failed build test is related to the latest tf release.