Testing and refactoring meta-issue

[Pbellive]

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:

• Refactoring
  • Generate coverage numbers from as many examples as we can (at least DC, time, frequency domain EM) discover unused code if it exists.
    • Looks like FEM code can go (JOcTree Reorganization #6)
    • Looks like we're on the fence about createQuadTreeFromImage (JOcTree Reorganization #6)
    • Do we need both regularizeOcTree and regularizeOcTree2? Yes but naming needs to be improved
    • Remove plotting stuff. Anything that's actually being used can go to jInvVis (JOcTree Reorganization #6)
  • Move any code that isn't OcTree specific to a more appropriate home.
  • Consolidate functionality into fewer files
  • Standardize call signatures re scalars vs. vectors.
  • Standardize what gets returned by edge, face, and nodal constraint functions

Testing:

• Sparse3 array creation/manipulation
• Mesh generation
  • createOcTreeFromBox
  • createOcTreeFromImage
  • createOcTreeFromSrcRec
• Differential operators
  • Curl
  • Gradient
  • Divergence
• Mass matrices
  • Face
  • Edge
  • Node
• Derivative of mass matrices
  • Edge
• Constraint matrices
  • Face
  • Edge
  • Node

[lruthotto]

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:

• createQuadTreeFromImage (nice to keep if we had some more 2D support, otherwise could go for the sake of clarity)
• regularizeOcTree vs. regularizeOcTree2 (what is the difference?)
• plotOcTreeMesh.m (used by anyone? If not, let's delete it.)
• one thing I find confusing when using this package is that sometimes arguments are given as arrays and sometimes element-wise (e.g., compare createOcTreeFromBox and OcTreeMeshFEM)

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

[dwfmarchant]

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]

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]

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]

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]

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 ***@***.***> 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 <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEDYB9-QqN25iQQjJ9IAcQHmaswJWumlks5rkPdZgaJpZM4MYeit>.

[dwfmarchant]

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]

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]

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