Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(get/set data record): Updated get_data/set_data functionality and new get_record/set_record methods #1568

Merged
merged 2 commits into from Oct 6, 2022

Conversation

spaulins-usgs
Copy link
Contributor

@spaulins-usgs spaulins-usgs commented Oct 4, 2022

New feature for MFArray and MFList data objects. These objects now have get_record and set_record methods that allow the user to get a set data including the data's settings (filename, binary, factor, iprn). Behavior of set_data has been changed to set data without changing the data's settings, except when set_data is passed a dictionary containing filename, binary, factor, or iprn, in which case it gives a deprecation warning and sets the data and the data's settings.

…ts have get_record and set_record method that allow the user to get a set data with all metadata. Behavior of set_data has also been changed to set data without changing the metadata. When set_data is passed metadata it gives a deprecation warning and sets the data and metadata.
@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Merging #1568 (7f3be17) into develop (14d69cb) will increase coverage by 0.0%.
The diff coverage is 76.0%.

@@           Coverage Diff            @@
##           develop   #1568    +/-   ##
========================================
  Coverage     72.4%   72.5%            
========================================
  Files          251     251            
  Lines        54216   54454   +238     
========================================
+ Hits         39300   39517   +217     
- Misses       14916   14937    +21     
Impacted Files Coverage Δ
flopy/mf6/data/mfdataarray.py 62.3% <61.4%> (+0.8%) ⬆️
flopy/mf6/data/mfdatalist.py 72.9% <73.6%> (-0.2%) ⬇️
flopy/mf6/data/mfdatastorage.py 70.6% <84.9%> (+1.2%) ⬆️
flopy/mf6/mfpackage.py 64.3% <100.0%> (+<0.1%) ⬆️
flopy/utils/datautil.py 63.4% <100.0%> (-0.5%) ⬇️
flopy/plot/map.py 82.3% <0.0%> (-1.4%) ⬇️
flopy/utils/gridgen.py 84.2% <0.0%> (-0.3%) ⬇️
flopy/modflow/mf.py 68.4% <0.0%> (ø)
flopy/mf6/mfmodel.py 56.5% <0.0%> (ø)
... and 11 more

@langevin-usgs
Copy link
Contributor

@spaulins-usgs, we probably need to do quite a bit of documenting for this new capability. Are there plans to add that to this PR?

Copy link
Contributor

@jdhughes-usgs jdhughes-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few issues with this PR:

  1. The PR message does not really convey the changes. I find use of "metadata" confusing.
  2. It would be nice if the testing of the get_data set_data capabilities were not imbedded within an already too long test. I would suggest having a specific test that works with a small test model and probably in a separate test file for mf6 core functionality.
  3. I wish you could get to values with a . (like blah.iprn) instead of with a [key]
  4. I do not think the PR should be accepted without documentation on how to do to get a set values. Maybe a brief tutorial based on the autotest.

@spaulins-usgs
Copy link
Contributor Author

spaulins-usgs commented Oct 4, 2022

@langevin-usgs, @jdhughes-usgs, I will then update the existing tutorials, "Working with MODFLOW List Data" and "Working with MODFLOW Grid Array Data" to reflect these changes.

@jdhughes-usgs, I changed the PR message. Hopefully it is less confusing now. I am now working on moving the testing to a new test. Regarding getting values with "." (as attributes of an object), I am not convinced that is better than the way it works now. There is some precedent for getting values/settings/metadata in a dictionary, for example JSON. Most python developers are already familiar with data/metadata in this format. Also note that this dictionary format can already be used to construct new data with filename/binary/iprn/factor settings.

@jdhughes-usgs
Copy link
Contributor

@spaulins-usgs I might suggest a smaller specific tutorial rather than add it to an existing tutorial. @langevin-usgs not sure if you agree?

@spaulins-usgs
Copy link
Contributor Author

@jdhughes-usgs Another option is to modify the "MODFLOW 6: Data Storage Information - How and Where to Store MODFLOW-6 Data" tutorial. This is a short tutorial that already focuses on setting the data records (filename, binary, factor, iprn) when building packages. I could add a few examples of getting and setting data records with get_record and set_record and this tutorial would still be one of our shortest tutorials.

…moved get/set record testing to its own test case
@spaulins-usgs
Copy link
Contributor Author

@jdhughes-usgs @langevin-usgs I have made the requested changes (1, 2, and 4) and the PR is ready to go. I will merge the PR shortly unless I hear otherwise.

@spaulins-usgs spaulins-usgs merged commit 984227d into modflowpy:develop Oct 6, 2022
wpbonelli pushed a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
…d new get_record/set_record methods (modflowpy#1568)

* feat(get/set data record): New feature where MFArray and MFList objects have get_record and set_record method that allow the user to get a set data with all metadata.  Behavior of set_data has also been changed to set data without changing the metadata.  When set_data is passed metadata it gives a deprecation warning and sets the data and metadata.

* feat(get/set record): Updated tutorial to include get/set record and moved get/set record testing to its own test case
wpbonelli pushed a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
…d new get_record/set_record methods (modflowpy#1568)

* feat(get/set data record): New feature where MFArray and MFList objects have get_record and set_record method that allow the user to get a set data with all metadata.  Behavior of set_data has also been changed to set data without changing the metadata.  When set_data is passed metadata it gives a deprecation warning and sets the data and metadata.

* feat(get/set record): Updated tutorial to include get/set record and moved get/set record testing to its own test case
wpbonelli pushed a commit that referenced this pull request Dec 14, 2022
…d new get_record/set_record methods (#1568)

* feat(get/set data record): New feature where MFArray and MFList objects have get_record and set_record method that allow the user to get a set data with all metadata.  Behavior of set_data has also been changed to set data without changing the metadata.  When set_data is passed metadata it gives a deprecation warning and sets the data and metadata.

* feat(get/set record): Updated tutorial to include get/set record and moved get/set record testing to its own test case
wpbonelli pushed a commit to wpbonelli/flopy that referenced this pull request Dec 14, 2022
…d new get_record/set_record methods (modflowpy#1568)

* feat(get/set data record): New feature where MFArray and MFList objects have get_record and set_record method that allow the user to get a set data with all metadata.  Behavior of set_data has also been changed to set data without changing the metadata.  When set_data is passed metadata it gives a deprecation warning and sets the data and metadata.

* feat(get/set record): Updated tutorial to include get/set record and moved get/set record testing to its own test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants