XDMF

[KyleVaughn]

Adds XDMF mesh type and file type. Allows for reading and writing of 2D XMDF mesh files.

[KyleVaughn]

Note that XML element members were made public because the PRIVATE attribute was applied selectively, leaving things like content and name public despite having setName or getContent and because the setters and getters are largely unnecessary. For instance, getParent is just a call to parent => thisXMLE%parent and getContent is just content=thisXMLE%content.

[aarograh]

@thfolk @KyleVaughn I'm seeing a couple comments about DBC vs. exception handlers. First off, thanks for adding one or the other... something is always better than nothing! I figured I'd take a minute to write a note about picking between the two though in case it's relevant to the discussion going on here:

• Exception Handling: Use this for bad conditions that could potentially be caused by user input. For example, two input options are related to each other: input A can only have certain values depending on what value input B has. That situations warrants a descriptive error message to provide feedback to the user. We often can't reasonably test all input combinations during input processing, so we need to provide feedback downstream too.
• DBC: I think of this more as feedback to the developer. In other words, if a DBC statement trips, then that probably means the developer called the routine in a way that it should never be called. It should be rare for a user to be able to cause a DBC statement to appear, since "Design by Contract" really refers to how we as developers tie various functions and classes together. If the developer obeyed the "contract", the user should never see this. This is also partly why we disable DBC checks in release builds, since they shouldn't be encountered by users anyway.

I haven't looked in detail at this code so I don't know which of these is appropriate (maybe both are appropriate in different places), but hopefully that helps in deciding whether or not things should be changed from one to the other.

[KyleVaughn]

Thanks @aarograh @thfolk ! It looks like I should change a decent amount of the DBC to exception handling, since it's primarily applied to assumptions about valid XMDF, valid HDF5, and what structure the mesh file will have.

[thfolk]

Thanks @aarograh. I'm saving your comment to keep in the memory bank!

[KyleVaughn]

I believe the tests are failing now due to CASL Trilinos being moved.

git clone https://github.com/CASL/Trilinos.git
Cloning into 'Trilinos'...
remote: Invalid username or password.
fatal: Authentication failed for 'https://github.com/CASL/Trilinos.git/'
The command "git clone https://github.com/CASL/Trilinos.git" exited with 128.

[yl5]

Overall looks good, but I'd suggest to reconsider deriving the type from the BaseFileType and rewriting the unit tests.

[KyleVaughn]

@yl5 Addressed all of your comments except for making XML element's children attribute private again. I need XML elements to persist in memory after leaving a subroutine, so making an allocatable XML element array, setting a pointer to it, then using the XML setChildren method isn't sufficient, since it just does thisXMLE%children => children and the underlying XML elements will be deallocated once leaving the subroutine, so the pointer persists longer than the target. I could potentially write a function just to allocate the children, then use getChildren to work with children one by one, but this goes back to the point of unnecessary functions. I don't think that hiding these attributes makes sense, since the XML class does not have any derived classes, so there is no benefit to writing setters, getters, etc. to provide a general interface to the object. I'm fine with leaving the other attributes private, since they have the methods they need to be useful for the most part, but not being able to access children is a real hindrance to working with the type.

[HendersonSC]

@yl5 Addressed all of your comments except for making XML element's children attribute private again. I need XML elements to persist in memory after leaving a subroutine, so making an allocatable XML element array, setting a pointer to it, then using the XML setChildren method isn't sufficient, since it just does thisXMLE%children => children and the underlying XML elements will be deallocated once leaving the subroutine, so the pointer persists longer than the target. I could potentially write a function just to allocate the children, then use getChildren to work with children one by one, but this goes back to the point of unnecessary functions. I don't think that hiding these attributes makes sense, since the XML class does not have any derived classes, so there is no benefit to writing setters, getters, etc. to provide a general interface to the object. I'm fine with leaving the other attributes private, since they have the methods they need to be useful for the most part, but not being able to access children is a real hindrance to working with the type.

I would claim that there are multiple reasons for a setter/getter and there are many reasons for one to exists. Further its existence and/or relevance is wholly unrelated to its internal logic (or lack thereof).

To the benefit of a developer some setFoo or getBar tied to a private data component, provides an absolute guarantee that you know where the data is changing if it changes. Since you know where it changes, during debugging sessions you can know when it changes, which inevitably leads to why it changes. Which makes things way easier than hunting for where a variable changes.

Take note, that I haven't reviewed the underlying code, just agreeing with @yl5. It is plausible that it needs to be public, but the reason it needs to be public can not be that the appropriate setter/getter is too simple. If it is just the technical problem of a pointer outliving target or I have somehow missed something, feel free to email me and/or ping me on slack.

[KyleVaughn]

@yl5 Talked with Brendan, and I'm going to keep it private. Will update later today.

[yl5]

@KyleVaughn, just a couple things as I saw your updates:

1) If you agree that XDMF is not a file type, maybe change the module and source file names as well. Maybe XMDFDataType or XMDFDataLibrary?

2) I might not be clear about the unit tests. The purpose is to test each subroutine independently to make sure we get expected outputs from the input parameters. We should write a SUBTEST for each type-bound or public procedure as the minimum. Within each SUBTEST, you can test different problem conditions (by varying the input parameters) to thoroughly cover the codes in that procedure, and this is where you may use the COMPONENT_TEST. The current implementation seems to cover the ballpark of the testing but you will need to breakdown/restructure it to test each procedure independently. For an integration test of multiple routines and the overall feature, that is normally what a regression test does.

[KyleVaughn]

@yl5 Renamed the module to XDMFMesh and removed the XDMFFileType, so import and export are simply importXDMFMesh and exportXDMFMesh.

The unit tests were rewritten to maintain independence as best as possible. Functions are used in other tests once that test has passed, like the clear routine and assignment operator. The export tests use the import subroutine to verify, which I'm not happy about, but setting up all the meshes that should be tested by hand would be tedious and a lot of code. Importing the exported meshes also allows me to access the written data easier.

The import and export now have component tests for the various cases that test the major branches in logic, as denoted by the table in each function.

Lastly, the XML children attribute was made private again.