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

refactor(crs): provide support without pyproj, other deprecations #1850

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Jun 29, 2023

This PR is is a continuation of #1737 which has a few broad aims:

  • Use a crs parameter, which can take an EPSG integer, a PROJ string, or a CRS instance from pyproj. This single parameter replaces (and deprecates) epsg, proj4 and proj4_str parameters, where present.
  • Use a prjfile parameter as a path to a .prj file that describes a coordinate reference system, which replaces (and deprecates) prj, where present.

This PR embeds these changes further within the code.

However, a vital change in this PR is to provide some support without pyproj installed. For instance, a modelgrid can be created using (e.g.) crs="EPSG:26915" or crs=26915, and the resulting object will always have a .epsg property with an integer code. This was the case before #1737, but was broken.

Another important remark for flopy without pyproj, it is recommended to use (e.g.) crs=26915 as a parameter, but the .crs attribute is always None, since this this only returns a pyproj CRS instance.


If this is too late to make it into flopy 3.4.0, the deprecation notices will be adjusted.

@mwtoews mwtoews requested a review from aleaf June 29, 2023 00:28
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #1850 (384f2ba) into develop (736c5b8) will increase coverage by 0.0%.
The diff coverage is 66.0%.

@@           Coverage Diff            @@
##           develop   #1850    +/-   ##
========================================
  Coverage     72.1%   72.2%            
========================================
  Files          255     257     +2     
  Lines        56108   56397   +289     
========================================
+ Hits         40499   40737   +238     
- Misses       15609   15660    +51     
Impacted Files Coverage Δ
flopy/discretization/structuredgrid.py 46.9% <ø> (ø)
flopy/discretization/unstructuredgrid.py 84.6% <ø> (ø)
flopy/discretization/vertexgrid.py 82.6% <ø> (ø)
flopy/mf6/mfmodel.py 58.7% <ø> (-0.5%) ⬇️
flopy/utils/mfreadnam.py 34.0% <33.3%> (ø)
flopy/export/shapefile_utils.py 82.4% <53.3%> (-5.5%) ⬇️
flopy/export/utils.py 64.7% <60.0%> (-0.2%) ⬇️
flopy/discretization/grid.py 74.6% <66.4%> (-3.3%) ⬇️
flopy/modflow/mfdis.py 84.5% <77.7%> (-1.1%) ⬇️
flopy/utils/crs.py 88.0% <84.2%> (-3.2%) ⬇️
... and 2 more

... and 19 files with indirect coverage changes

@wpbonelli
Copy link
Member

@mwtoews 3.4.0 release is underway in support of mf6.4.2.

The mf6 CI failure was caused by a file missing from the initial mf6.4.2 distribution, since fixed. The docs failure looks like a GitLab outage

@mwtoews
Copy link
Contributor Author

mwtoews commented Jun 29, 2023

I thought this one was cutting a bit short-of-time, so it can wait for the next one. I can adjust these notices to:

        .. deprecated:: 3.4.0
           The following keyword options will be removed for FloPy 3.5:

(assuming 3.5 is the next development version). I'll also modify messages from #1737 to be "removed for FloPy 3.5" too, as these keyword options are related, and will make for easier effort while eventually removing these options.

@jdhughes-usgs
Copy link
Contributor

@aleaf could you take a look at this PR?

Copy link
Contributor

@aleaf aleaf left a comment

Choose a reason for hiding this comment

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

Hi @mwtoews, sorry for the tardiness in getting to this. This looks good, thanks for restoring this functionality and for all of the code cleanup as well. Just one practical question- how do we ensure follow-through on all of the deprecations during the next minor release? I could see them being easy to miss. How do other projects handle this? Do we need to add searching for deprecations or something similar to our release checklist? Thanks.

@@ -72,8 +94,6 @@ class Grid:
bottom elevations of all cells
idomain : int or ndarray
ibound/idomain value for each cell
crs : pyproj.CRS
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing crs from the attributes documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was moved further down to the property, which would render here (currently blank). With Sphinx it is cross-referencable using :py:attr:`crs. I might get around to moving the other property docs later, since it's easier to see and xref property docs beside the relevant code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, good to know- I figured that was probably a better way to do it.

@mwtoews
Copy link
Contributor Author

mwtoews commented Jul 18, 2023

The last task that I'll get to is to increment the deprecations to 3.6. Update: this is now done.

As for how to follow-up acting on them, I'm not aware of any automated process. It could be done using searching with (e.g.) git grep "deprecated::", probably the best time to do this is at the start of a new development cycle. So now is a perfect time to review these.

@mwtoews mwtoews merged commit 02fae7d into modflowpy:develop Jul 19, 2023
@mwtoews mwtoews deleted the without-pyproj branch July 19, 2023 04:16
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.

4 participants