Separate BMI wrapper from core model #19
Conversation
In this commit, I went through. and tried to find all the things in the core model that are wrapped and accessed by the BMI wrapper. In the process, I tried to take things that didn't need to be in the wrapper, and instead put them in the main model. The biggest thing like this that I found were the model time, which is critical for the model, but was defined both in the BMI wrapper and in the core model itself. The other thing was a list of input and output variable names, that is used to parse the yaml file that specifies the run. I moved all these things into the core model, under the premise that if the model is instantiated from a BMI framework, it will acquire those processes once it is initialized, and doesn't need them before initialization.
We would like to see that the model can run alone and also with a BMI interface, so I wrote a new script which runs the model as if it were a normal model, and then also as a BMI model.
File import is a core initialization function, and it should be with the other initialization methods in that class, not with the other tools.
Also change script to only import the core model and only update for one timestep to test for runtime errors Add output files to gitignore
|
just added a few small changes to fix an import error and keep output files from accumulating in the git log. |
|
I suspect the double variables existed so that the BMI-wrapper held all of the variables that are expected for the model to be BMI-compliant so someone less familiar with DeltaRCM but knowledgeable with regards to the BMI scheme could easily access those variables. But if this is the first step to separating the core DeltaRCM model from the BMI-wrapper (which I think is worthwhile) then I support it. |
|
@ericbarefoot can you change the requirement in Want to make sure tests pass. More thoughts coming. EDIT: you need to change it here too |
Hmm. Can you elaborate on that? The only attribute that was doubly defined was |
|
Cool this is something I definitely think we should do, so I'm in support of the idea, but I have some thoughts. I think separating the two in this repo into two classes, is a good first step. In the long run, I think that this project To this end, I think moving the dictionary of BMI variable names into pyDeltaRCM is wrong. In fact, I would support removing the long tuples and dicts defined at the top of A good thing to try to make sure we get this right is to write a few simple tests. One where you initialize pyDeltaRCM directly from the |
|
Eric, I see what you are saying. I guess I mean keeping the bmi variables separate in the wrapper and passing them to the corresponding model variables was intentional. Likely in-line with the BMI 'best practices' https://bmi.readthedocs.io/en/latest/bmi.best_practices.html#best-practices Which suggests leaving internal model variable names unchanged, but provided a BMI 'standard' name which maps to the internal name. |
|
@ericbarefoot the scripts you put in look basically like tests! Can you wrap them into tests for real though? Or let me know if you want me to do it? Not sure if you saw my edit above, you need to change the |
I have no idea how to do this! I can change the bmipy call, but I do not know what you mean when you say wrap it into tests. |
@elbeejay this makes a lot of sense, but I'm not quite sure how to resolve the issue that the yaml file uses these dictionaries to parse in the core model run itself. If we leave it the original way, the model actually can't run without the BMI wrapper. |
Then we should change the The BMI wrapper should be a wrapper, defining the things it needs (like a dictionary to map BMI variables to pyDeltaRCM variables) in its own world. This will be a breaking change to the API for sure, but I think it's a good move... |
Groovy, thanks for the thoughts and feedback all. This will probably require a little more effort and time then, so I'll close this PR, and re-open when I've repaired the |
|
Okay that sounds good, let me know when you've got something workable in your repo branch and we can chat about tests / I can write a few. |
|
OK, I made a few linting changes, and added a tiny bit of documentation @amoodie, but I will defer the new testing till later, if you don't mind, since that seems like more of a project. |
|
Okay, do you mind if I write some tests for it? I'll work on it this afternoon and send your way. |
… default, then in init_tools, we only load it if it is given. If a bad object is given, we let pyyaml handle that with a TypeError as usual.
Change logic, add error message, add tests
… into separate_bmi
BUG@ericbarefoot have you tried running this from outside the repository? I think that this line with I fixed this locally with: self._file_dir = os.path.realpath(os.path.dirname(__file__))
self.default_file = os.path.join(self._file_dir, 'default.yml')Figured it may be easier for you to just implement this and push it than for my to PR again. |
I have done this, and pushed again. I agree that anything that depends on knowing which directory the user is in is bad, but what is the good python solution for this? In |
Bug fix, tests fixes
… into separate_bmi
|
Ok now that this works for everyone, I'll go ahead and merge this! As of now, BMI development will be separate from model development, with the eventual goal of separating the BMI into a different repository entirely. |
|
🎉 thanks @ericbarefoot |
Separate BMI wrapper from core model
One of the themes that keeps coming up in our discussions is the confusion caused by the BMI wrapper being so intertwined with the workings of the core model. I went through and tried to concretely identify all the places where the core model requires some component from the wrapper, or where the two doubly define a parameter. In both cases, I deferred to the core model, shunting lists that were required by both to the core model, and modifying the wrapper so it accesses the needed parameters from the model instance.
The two key things that had something like this were
_timewhich was defined both when the BMI delta was initialized, and again re-set when the pyDeltaRCM delta object was initialized.I also made a new script, so that the two should be able to run separately
Thoughts are appreciated. This relates partly to the question of whether to separate into two repos, but is really just a first step towards that.