search

JuliaInv / JOcTree

Other
4 stars 7 forks source link

Testing and refactoring meta-issue #5

Open Pbellive opened 7 years ago

Pbellive commented 7 years ago

JOcTree needs testing. Before adding unit tests, it might be best to start with a simple refactoring, including pruning old unused functionality and consolidating the code into fewer files. Obviously, broad consensus would be required to remove any code from the package. As a first step I'll create a to-do list. Please post anything that should be added to the list.

Refactoring:

Testing:

lruthotto commented 7 years ago

Thanks for getting this started. I totally agree that refactoring and testing is needed here. In addition to the above items, I suggest we also look at:

We could also use this cleanup and add some (even if only minimal) documentation to functions we touch.

dwfmarchant commented 7 years ago

Thanks for this Patrick.

I think we can start by getting rid of some stuff that isn't being used anymore. createQuadTreeFromImage is one. I also believe that all of the FEM stuff is outdated and is not being used anymore - I talked to @eldadHaber and @cschwarzbach about this and I think the consensus was that it can be deleted. I set up a new package for the visualization code and copied plotOcTreeMesh over to that. Tested it and it still works fine, but it can go from here now.

How does everyone feel about moving the 'utility code' into a couple of submodules? I was thinking that all of the octree creation code in one, as well as the IO code (export/importOcTreeMeshRoman from MaxwellUtils) in another.

I'd also like to combine a number of the short files so there are a few less files in the directory. For example all of the numbering codes could get combined.

I'd be happy to start working on this tomorrow if you'd like but I could also wait until after our meeting next week.

Pbellive commented 7 years ago

Thanks for the input Dave and Lars. I've added your items to the list in the initial comment. Dave, if you'd like to get started tomorrow I have no problem. Seems like there's a chunk of totally uncontroversial work like removing the FEM and plotting stuff.

I think it makes sense to remove createQuatTreeFromImage unless someone's going to add more 2D functionality. I think that would be a fun project but I'm not able to take it on now.

Other stuff can be discussed over Skype next week.

lruthotto commented 7 years ago

Thanks for updating the list Patrick.

@dwfmarchant : I'm a little hesitant about splitting up the code into even more submodules (we already have quite a number of packages here). Let's discuss this next week. The other suggestions are not controversial and you can go ahead tomorrow if you like.

cschwarzbach commented 7 years ago

Re: regularizeOcTree vs. regularizeOcTree2

regularizeOcTree refines an OcTree mesh such that each OcTree cell only has face neighbours that are of the same size, that are half the size or that are twice the size. regularizeOcTree2 refines an OcTree mesh such that each OcTree cell only has nodal neighbours that are of the same size, that are half the size or that are twice the size. In addition, regularizeOcTree2 checks that each OcTree cell has either neighbours that are half the size or neighbours that are twice the size, not both. This ensures a more conservative mesh coarsening. Pairs of cells are face neighbours or nodal neighbours if they share a face or a node.

regularizeOcTree2 is an essential part of OcTree mesh generation. Some functions such as getNodalConstraints, getEdgeConstraints and getFaceConstraints rely on the regularity conditions that regularizeOcTree2 enforces.

regularizeOcTree2 requires regularizeOcTree. From the interface point of view, regularizeOcTree2 is the function that should be called during mesh generation. To make things more intuitive, we could rename regularizeOcTree2 to regularizeOcTree and move the current code in regularizeOcTree inline or to a private function with a different name.

lruthotto commented 7 years ago

Thanks for clarifying this, Christoph. Could we add this description as a docstring to the code? I also think that renaming might help to avoid the confusion.

On Mar 10, 2017, at 2:10 AM, Christoph Schwarzbach notifications@github.com wrote:

Re: regularizeOcTree vs. regularizeOcTree2

regularizeOcTree refines an OcTree mesh such that each OcTree cell only has face neighbours that are of the same size, that are half the size or that are twice the size. regularizeOcTree2 refines an OcTree mesh such that each OcTree cell only has nodal neighbours that are of the same size, that are half the size or that are twice the size. In addition, regularizeOcTree2 checks that each OcTree cell has either neighbours that are half the size or neighbours that are twice the size, not both. This ensures a more conservative mesh coarsening. Pairs of cells are face neighbours or nodal neighbours if they share a face or a node.

regularizeOcTree2 is an essential part of OcTree mesh generation. Some functions such as getNodalConstraints, getEdgeConstraints and getFaceConstraints rely on the regularity conditions that regularizeOcTree2 enforces.

regularizeOcTree2 requires regularizeOcTree. From the interface point of view, regularizeOcTree2 is the function that should be called during mesh generation. To make things more intuitive, we could rename regularizeOcTree2 to regularizeOcTree and move the current code in regularizeOcTree inline or to a private function with a different name.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/JuliaInv/JOcTree/issues/5#issuecomment-285595134, or mute the thread https://github.com/notifications/unsubscribe-auth/AEDYB9-QqN25iQQjJ9IAcQHmaswJWumlks5rkPdZgaJpZM4MYeit.

dwfmarchant commented 7 years ago

I would also like to move the code that reads and writes UBC octree model and mesh files from MaxwellUtils to jOcTree.

There is also some functions for exporting to VTK in MaxwellUtils - Are they still being used? If so they could move here as well.

Pbellive commented 7 years ago

I agree that the OcTree IO code needs to be moved. I'm not sure JOcTree is the best place. How would you guys feel about creating an IO submodule in jInv? For now I think this would just contain stuff for reading and writing UBC data files but in the future I could see it containing utilities for saving/loading various jInv related data using the JLD binary format, or for reading in data from various third party sources---tetgen files for tetrahedral meshes for example.

As for the VTK stuff, I use it regularly but it's fine with me if you remove it from MaxwellUtils. It's not a good home for it. I was thinking it could live in jInvVis. I don't know if anyone but me uses it. If you just delete it, then I can add documentation and make a pr to jInvVis. It has a bunch of dependencies which it wouldn't make sense to load every time JOcTree is loaded.

lruthotto commented 7 years ago

@dwfmarchant has done some of the listed things with the PR I just merged. Thanks, Dave you may tick some boxes above :)