modflowpy / flopy

A Python package to create, run, and post-process MODFLOW-based models.
https://flopy.readthedocs.io
Other
517 stars 313 forks source link

rotation bust #161

Closed jtwhite79 closed 7 years ago

jtwhite79 commented 8 years ago

Just realized that the rotation in spatialReference is actually positive clockwise, not positive counterclockwise like PEST (and the docstrings). So the question is do we change the code so that it is in sync with PEST or change the docstring (and add a note about this difference). I'm inclined to change the code so that flopy is the same as PEST but this will break older scripts.

Does anyone have any objections? or even care?

mbakker7 commented 8 years ago

Any idea on how many people will have actually used this?

Maybe add a deprecation Warning that shows on the screen that the behavior has changed as of immediate to comply with the doc string and PEST?

Mark

On Tue, Oct 11, 2016 at 8:53 PM, jtwhite79 notifications@github.com wrote:

Just realized that the rotation in spatialReference is actually positive clockwise, not positive counterclockwise like PEST (and the docstrings). So the question is do we change the code so that it is in sync with PEST or change the docstring (and add a note about this difference). I'm inclined to change the code so that flopy is the same as PEST but this will break older scripts.

Does anyone have any objections? or even care?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/modflowpy/flopy/issues/161, or mute the thread https://github.com/notifications/unsubscribe-auth/ABTNyCvfMYnUzCVoGVAx_C1XIgBDNyIfks5qy9segaJpZM4KT-fm .

aleaf commented 8 years ago

I agree with Mark's comment. It will break some of my scripts, but nothing that isn't easy to fix. FYI, I checked the OGW archiving instructions and they don't say anything about what direction the rotation should be in.

jtwhite79 commented 8 years ago

Done - added warnings if rotation != 0.0. But...of course it can't be that easy - t31_test.py test_get_destination_data() is failing. Looks like there are some "known" MODPATH results using the old rotation scheme, which is now broken. I commented out the failing assertions.

Andy - can you look at this?

aleaf commented 8 years ago

oh bummer. Will do.

jtwhite79 commented 8 years ago

Thanks Andy!

langevin-usgs commented 8 years ago

Andy, this is still broken. If you run the flopy3_MapExample notebook, you'll see that it is rotating around the wrong point. The default should be the upper left corner as the rotation point. And even if you specify xul and yul in map constuctor, I'm not sure things are working correctly.

image

aleaf commented 8 years ago

Chris, I did have the sign flipped around in the conversion between xll and xul (fixed here). test_rotation() should demonstrate that SpatialReference doesn't care about the rotation point. Regardless of whether the ul or ll corners are supplied, the real corner of the grid will be at those points. I think the map constructor is simply showing the map in model coordinates (without any offset; same as the global coordinates output by modpath). Do we want an option to plot in real world coordinates instead?

aleaf commented 8 years ago

is there any reason for this code in ModelMap.__init__?

# model map override spatial reference settings
        if xul is not None:
            self.sr.xul = xul
        if yul is not None:
            self.sr.yul = yul
        if rotation is not None:
            self.sr.rotation = rotation

seems like it might be easier to just call SpatialReference.set_spatialreference() or create a new SpatialReference instance with the supplied arguments. Right now the the xll and yll attributes (which are used for the offset) are not getting set. Likewise there is no scaling being applied if the model has different length units than the real world coordinate system.

langevin-usgs commented 8 years ago

Andy, can you try adding a call to SpatialReference.set_spatialreference(). I think that would be easiest. We definitely want modelmap to show things in real coordinates (not model coordinates).