Provide a data reader for UGRID files #2845
Conversation
…nate v variables. Previously a variable was identified as a coordinate variable only if it was referenced by a "data" variable. Now if a variable can be identified as a coordinate variable via the CF specification it is classified as a coordinate variable. Also simplified the DCCF class assignment of coordinate variables.
simple algorithm fails :-(
…s and time steps were stored in different NetCDF files. E.g. one variable, one time step per file.
range -180..180, and removed various hacks to handle this situation.
| //! generated name is simply the join of the elements of \p s | ||
| //! using 'x' as a delimiter | ||
| // | ||
| static string MakeMeshName(std::vector<string> s); |
There was a problem hiding this comment.
Why does this function have to be static?
There was a problem hiding this comment.
So it can be used without instantiating a class as is done in DCCF.cpp
There was a problem hiding this comment.
This this the use case that is required occasionally: a helper function that takes input and produces an output, and the operation is not necessarily associated with a particular class.
There are at least 2 ways to achieve the goal: 1) make it a function under a particular namespace. E.g.: many functions under the std:: namespace in C++. 2) declare the function static and insert it into a particular class.
Way 2) is a little messy, because the static functions will out-live the class object itself, which is really against the core ideas of object oriented programming. Way 1) is adapted by the standard C++ to meet this particular requirement, so I suspect it's got other advantages.
I suggest adopting Way 1) here, but it's ultimately the decision of the DC author.
There was a problem hiding this comment.
This method has no purpose that isn't associated with the DC class (i.e. it will NOT out-live the class. That is why it was put into the same namespace. Putting it in some other made up namespace would only cause confusion. I'm inclined to leave it where it is.
There was a problem hiding this comment.
From a design perspective, if all purposes of a function is associated with a particular class, then wouldn't it make sense to have this function be a member function of that class?
There was a problem hiding this comment.
It is a member function of a class. It's a static member function. One of the intended use cases for static member functions is to allow their use without instantiating the class.
| namespace { | ||
| vector<string> make_unique(vector<string> v) | ||
| { | ||
| sort(v.begin(), v.end()); |
There was a problem hiding this comment.
STD has a function named make_unique so I don't think it's a good idea to make your custom function the same name.
|
@sgpearse this PR is failing the data manager regression tests. From what I can tell it is because this version of the code reports an additional coordinate variable, "zf", in the file 24Maycontrol.04000.000000.nc. The new behavior is correct. Not sure how to go about updating the baselines after this PR is accepted. |
|
|
||
| int initMesh(NetCDFCFCollection *ncdfc, std::map<string, DC::Mesh> &meshMap) override; | ||
|
|
||
| string getMapProjection() const override { return ("+proj=eqc +ellps=WGS84 +lon_0=0.0 +lat_0=0.0"); } |
There was a problem hiding this comment.
How about we put the function implementation in the .cpp file?
Otherwise, making a change to the function body in the header file will cause a re-compilation of almost the entire project.
There was a problem hiding this comment.
Putting inexpensive getters in .h files is done throughout the code base, and widely done in practice. What else would be gained by moving the definition other than saving a few microseconds (at most) during compilation?
There was a problem hiding this comment.
I'm afraid the save is more than microseconds.
-- With implementation being in the header, every file that includes this header will be re-compiled. That's almost the entire VAPOR, and that compilation is in the order of minutes.
-- With implementation being in the body, only DC is re-compiled and everybody else just need to re-link, which takes microseconds.
Plus, it really makes the header look tidy and easier to read when you hide details away.
There was a problem hiding this comment.
I'm afraid the save is more than microseconds.
-- With implementation being in the header, every file that includes this header will be re-compiled. That's almost the entire VAPOR, and that compilation is in the order of minutes.
-- With implementation being in the body, only DC is re-compiled and everybody else just need to re-link, which takes microseconds.
I don't believe that the compilation time will change significantly if the definition is moved. You'll have to do an experiment and convince me with the results :-)
Plus, it really makes the header look tidy and easier to read when you hide details away.
That's your personal opinion.
@shaomeng I am not just being stubborn. I'm making a point. Changes requested by reviewers should have a meaningful impact to the code in terms of performance, readability, robustness, future-proofing, following best practices, or simply following our own conventions. I don't want developers to have to change their code simply because a reviewer has an opinion on how things should look that may or may not agree with the opinion of others. That just waists everyone's time and does not lead to better code.
There was a problem hiding this comment.
Regarding compilation time, maybe I'm not clear enough, but I'm not saying that the 1st time you compile the project there will be a significant difference. I'm saying that when somebody debugs the code, for example change the function implementation or simply insert a print statement, function body being in the header will trigger all the downstream files to be re-compiled again. That re-compile of all the downstream files cost minutes, and that cost is totally avoidable.
I can tell my personal experience debugging a crashing problem in the last release cycle. I needed to insert a few print statements in some grid classes. Guess how much time it takes to re-compile a few print statements? Minutes. Why? Almost the entire project is re-compiled, because almost every file includes a grid class.
There was a problem hiding this comment.
@shaomeng, I'm sorry but you haven't convinced me that there is a good reason to prohibit simple member functions from being defined in a .h file. There appears to be no practical reason for such a prohibition. I don't really believe that there is a need for debugging a function that does no more than return a literal of even a member variable., do you? You've mentioned having trouble debugging more complex functions defined in .h files. If so, I'd suggest submitting an issue, or even a PR, to move those troublesome functions. There also does not appear to be any consensus amongst the C++ community that I can see. You've claimed that every college class teaches not to do this, but that claim seems wholly unsubstantiated. The existence of a single example doesn't do much to support this argument. On the contrary I've identified three large, modern C++ development efforts that do not abide by any such convention (add a fourth, Intel OSPRay to the list as well). And, FWIW the Google C++ Style Guide recommends limiting functions defined in .h files to 10 lines of code.
If you really think the VAPOR code base would be improved by adopting a convention on this that we all abide by, you should propose it to the group as a change to our coding conventions. If you can convince the majority I am happy to comply (and will expect others to as well).
There was a problem hiding this comment.
I'm sorry John, I think the VAPOR coding convention has a clear specification, but my interpretation could be wrong. What do you think this sentence in it means?
Any public class should generally be declared in its own header file, and defined in its own source file.
If it doesn't mean every function body goes to .cpp files, I'm happy to learn what the guideline actually is regarding what to put in headers and in .cpp files.
(Note, what this specification means, and if real code abides to it, are two different issues.)
There was a problem hiding this comment.
Sam, there is a qualifier in the statement that may be leading to the confusion: "generally". In a context like this "generally" means that most of the time the rule applies, but "generally" provides the flexility to ignore it. Lacking specificity about when the rule can be ignored it is left up to the judgement of the reader.
There was a problem hiding this comment.
Well, talking about "generally." When judging how and when "generally" applies, my understanding is that there should be some strong and evident benefits to support an occasional break of the rule. That strong and evident benefit should be in the nature of enhancing the functionality, robustness, portability (and alike) of the program. One example of such scenario is templated function.
The justification to apply "generally," however, should not be: "I don't want to spend 5 extra seconds to abide to the rules." Because, that's almost the definition of abusing the flexibility provided by the rules. Do you think this is a reasonable understanding of "generality" in that sentence, and how to interpret rules in general?
Regard to this specific case, I haven't seen any benefit to the software that you provide to support this occasion of breaking the rule. If I missed it, maybe you could raise it again?
There was a problem hiding this comment.
@shaomeng I'm not debating this anymore. Since there does not appear to be any consensus from the C++ community if you want to ban all method definitions from .h files you'll need to convince the group to clearly state this in our coding guidelines.
Fixes #2714
This PR provides a data importer for UGRID files. At present the reader handles all of the supported UGRID topologies except "Fully unstructured 3D" (general 3D unstructured meshes are not supported internally by VAPOR).