gplately icon indicating copy to clipboard operation
gplately copied to clipboard

backwards compatibility `ridge_transforms`

Open michaelchin opened this issue 1 year ago • 20 comments

Discussed in https://github.com/GPlates/gplately/discussions/248

Originally posted by michaelchin August 5, 2024 @michaelchin - tests are failing now. To help with backwards compatibility you could retain the ridge_transforms attribute which is the combination of BOTH ridges and transform boundaries.

Originally posted by @brmather in https://github.com/GPlates/gplately/issues/243#issuecomment-2268085970

michaelchin avatar Aug 06 '24 08:08 michaelchin

  • bring back ridge_transforms attribute
  • bring back plot_ridges_and_transforms&get_ridges_and_transforms function
  • bring back misc_transforms attribute
  • bring back plot_misc_transforms&get_misc_transforms function
  • add warning messages

michaelchin avatar Aug 06 '24 08:08 michaelchin

Do we want to bring back misc_transform attribute (and related functions)?

We could probably skip it since it's now taken care of by transforms (which contains gpml:Transform).

jcannon-gplates avatar Aug 06 '24 08:08 jcannon-gplates

Do we want to bring back misc_transform attribute (and related functions)?

We could probably skip it since it's now taken care of by transforms (which contains gpml:Transform).

I guess it has the same backwards compatibility problem

michaelchin avatar Aug 06 '24 08:08 michaelchin

Maybe we should just remove it, and have it hard fail - it's a bit of a confusing attribute to keep around IMO? That would mean changing misc_transforms to transforms in the notebooks, etc, though.

jcannon-gplates avatar Aug 06 '24 08:08 jcannon-gplates

Oh, you probably meant keep it around just so you can emit a warning. Yeah that makes sense. And I guess eventually it could get removed. We can probably make it a deprecation warning then.

jcannon-gplates avatar Aug 06 '24 08:08 jcannon-gplates

  • misc_transforms attribute

Eventually both misc_transforms and ridge_transforms should be removed. There is no enough reason for them to stay and they may cause confusion. People may wonder why there is a function plot_ridges_and_transforms(). Can't I just plot_ridges(); plot_transforms()?

Anyway, just give users a grace period to change their code voluntarily. If they don't, we force them to do it in the future release.

michaelchin avatar Aug 06 '24 09:08 michaelchin

Maybe misc_transforms is a bit worse than ridge_transforms. But it seems to me both of them are too bad to stay.

That would mean changing misc_transforms to transforms in the notebooks, etc, though.

I think this is the reason why Ben wants to keep ridge_transforms. But maybe misc_transforms is not used as much as ridge_transforms. Frankly I don't see how hard it is to change them.

Anyway, let's be nice and give users a grace period .

michaelchin avatar Aug 06 '24 09:08 michaelchin

Sounds good. Deprecation warnings for stuff that will eventually be removed. And regular warnings for stuff whose meaning is changing. Then users can ignore them until something doesn't work as they expected ;-)

jcannon-gplates avatar Aug 06 '24 09:08 jcannon-gplates

  • make self.ridge_transforms a property
  • add warning message in ridge_transforms's getter method "Deprecated! The 'ridge_transforms' property will be removed in the next GPlately release. You need to update your workflow to use the 'ridges' and 'transforms' properties instead, otherwise your workflow will be broken by the next GPlately release."
  • add another warning message in ridge_transforms's getter method "The 'ridge_transforms' property has been changed since GPlately release 1.3.0. Now the 'ridge_transforms' property contains both gpml:Transform and gpml:MidOceanRidge features in the reconstruction model. You need to check your workflow to make sure the new 'ridge_transforms' property still suits your purpose. In the previous GPlately releases, the 'ridge_transforms' property contains only gpml:MidOceanRidge features."

michaelchin avatar Aug 08 '24 02:08 michaelchin

  • bring back plot_ridges_and_transforms()
  • add warning message "Deprecated! The 'plot_ridges_and_transforms' function will be removed in the next GPlately release. You need to update your workflow to use the 'plot_ridges' and 'plot_transforms' functions instead, otherwise your workflow will be broken by the next GPlately release."
  • add another warning message "The 'plot_ridges_and_transforms' function has been changed since GPlately release 1.3.0. Now the 'plot_ridges_and_transforms' function plots both gpml:Transform and gpml:MidOceanRidge features in the reconstruction model. You need to check your workflow to make sure the new 'plot_ridges_and_transforms' function still suits your purpose. In the previous GPlately releases, the 'plot_ridges_and_transforms' function plots only gpml:MidOceanRidge features."

michaelchin avatar Aug 08 '24 03:08 michaelchin

  • bring back get_ridges_and_transforms()
  • add warning message "Deprecated! The 'get_ridges_and_transforms' function will be removed in the next GPlately release. You need to update your workflow to use the 'get_ridges' and 'get_transforms' functions instead, otherwise your workflow will be broken by the next GPlately release."
  • add another warning message "The 'get_ridges_and_transforms' function has been changed since GPlately release 1.3.0. Now the 'get_ridges_and_transforms' function returns both gpml:Transform and gpml:MidOceanRidge features in the reconstruction model. You need to check your workflow to make sure the new 'get_ridges_and_transforms' function still suits your purpose. In the previous GPlately releases, the 'get_ridges_and_transforms' function returns only gpml:MidOceanRidge features."

michaelchin avatar Aug 08 '24 03:08 michaelchin

add warning message for plot_ridges() "The 'plot_ridges' function has been changed since GPlately release 1.3.0. Now the 'plot_ridges' function plots all gpml:MidOceanRidge features in the reconstruction model. You need to check your workflow to make sure the new 'plot_ridges' function still suits your purpose. In the previous GPlately releases, the 'plot_ridges' function plots only the ridges in the gpml:MidOceanRidge features(the transforms in the gpml:MidOceanRidge features are not plotted)."

michaelchin avatar Aug 08 '24 03:08 michaelchin

add warning message for plot_transforms() "The 'plot_transforms' function has been changed since GPlately release 1.3.0. Now the 'plot_transforms' function plots all the gpml:Transform features in the reconstruction model. You need to check your workflow to make sure the new 'plot_transforms' function still suits your purpose. In the previous GPlately releases, the 'plot_transforms' function plots only the transforms in the gpml:MidOceanRidge features(all the gpml:Transform features are not plotted)."

michaelchin avatar Aug 08 '24 03:08 michaelchin

  • make self.ridges a property
  • add warning message for the ridges property "The 'ridges' property has been changed since GPlately release 1.3.0. Now the 'ridges' property contains all the gpml:MidOceanRidge features in the reconstruction model. You need to check your workflow to make sure the new 'ridges' property still suits your purpose. In the previous GPlately releases, the 'ridges' property contains only the ridges in the gpml:MidOceanRidge features(the transforms in the gpml:MidOceanRidge features are not included)."

michaelchin avatar Aug 08 '24 05:08 michaelchin

  • make self.transforms a property
  • add warning message for the transforms property "The 'transforms' property has been changed since GPlately release 1.3.0. Now the 'transforms' property contains all the gpml:Transform features in the reconstruction model(the transforms in the gpml:MidOceanRidge features are not included). You need to check your workflow to make sure the new 'transforms' property still suits your purpose. In the previous GPlately releases, the 'transforms' property contains only the transforms in the gpml:MidOceanRidge features(the gpml:Transform features are not included)."

michaelchin avatar Aug 08 '24 05:08 michaelchin

add warning message for get_ridges() "The 'get_ridges' function has been changed since GPlately release 1.3.0. Now the 'get_ridges' function returns all gpml:MidOceanRidge features in the reconstruction model. You need to check your workflow to make sure the new 'get_ridges' function still suits your purpose. In the previous GPlately releases, the 'get_ridges' function returns only the ridges in the gpml:MidOceanRidge features(the transforms in the gpml:MidOceanRidge features are not plotted)."

michaelchin avatar Aug 08 '24 06:08 michaelchin

add warning message for get_transforms() "The 'get_transforms' function has been changed since GPlately release 1.3.0. Now the 'get_transforms' function returns all the gpml:Transform features in the reconstruction model(the transforms in the gpml:MidOceanRidge features are not included). You need to check your workflow to make sure the new 'get_transforms' function still suits your purpose. In the previous GPlately releases, the 'get_transforms' function returns only the transforms in the gpml:MidOceanRidge features(the gpml:Transform features are not plotted)."

michaelchin avatar Aug 08 '24 06:08 michaelchin

allow users to disable "ridge and transform" related warning

michaelchin avatar Aug 12 '24 01:08 michaelchin

allow users to disable "ridge and transform" related warning

No, we should annoy users so that they have the motive to update their code with the new API.

michaelchin avatar Sep 17 '24 05:09 michaelchin

Maybe we can find a RA to do this as well.

michaelchin avatar Sep 17 '24 05:09 michaelchin

@Hojat-Shirmard let's start with this one. I will send you an email with instructions later.

michaelchin avatar Oct 22 '24 05:10 michaelchin

use logger.debug() to print messages in plot_ridges(), plot_ridges(), get_ridges() , get_transforms(), self.ridges property and self.transforms property.

Just to avoid annoying too many people with the warning messages.

The warning messages in other functions are designed to be annoying so that people have the motivations to stop using them.

michaelchin avatar Oct 24 '24 04:10 michaelchin

Hi @Hojat-Shirmard

Do you want to read the comments below and add the code accordingly?

https://github.com/GPlates/gplately/issues/250#issuecomment-2274859508 https://github.com/GPlates/gplately/issues/250#issuecomment-2274882294 https://github.com/GPlates/gplately/issues/250#issuecomment-2274883414 https://github.com/GPlates/gplately/issues/250#issuecomment-2274887170 https://github.com/GPlates/gplately/issues/250#issuecomment-2274888945 https://github.com/GPlates/gplately/issues/250#issuecomment-2275008375 https://github.com/GPlates/gplately/issues/250#issuecomment-2275012714 https://github.com/GPlates/gplately/issues/250#issuecomment-2275014467 https://github.com/GPlates/gplately/issues/250#issuecomment-2275016017

michaelchin avatar Dec 09 '24 08:12 michaelchin

Hi @michaelchin I just added warning messages using logger.debug().

Hojat-Shirmard avatar Dec 10 '24 05:12 Hojat-Shirmard

Hi @michaelchin I just added warning messages using logger.debug().

Could you please double check the changes that you have made in https://github.com/GPlates/gplately/commit/89c8297ba70284d757b907997b7dc42e7fe1e340 ? It seems to me some of them are not quite right.

michaelchin avatar Dec 10 '24 06:12 michaelchin

For example, see the screenshot below. I couldn't make sense of it. What's the meaning of this?

Please take your time and no rush.

屏幕截图(10)

michaelchin avatar Dec 10 '24 06:12 michaelchin

@michaelchin I remember you emailed me and asked me to "Try to add a "ridge_transforms" attribute in plot.py"

Hojat-Shirmard avatar Dec 10 '24 06:12 Hojat-Shirmard

@michaelchin I remember you emailed me and asked me to "Try to add a "ridge_transforms" attribute in plot.py"

I don't understand this. It seems to me unrelated to our discussion now.

Have you tried to test your code changes in https://github.com/GPlates/gplately/commit/89c8297ba70284d757b907997b7dc42e7fe1e340?

Can you see the errors in https://github.com/GPlates/gplately/commit/89c8297ba70284d757b907997b7dc42e7fe1e340?

michaelchin avatar Dec 10 '24 06:12 michaelchin

@Hojat-Shirmard please slow down and take your time, try to understand the task and learn Python programming. Thank you.

michaelchin avatar Dec 10 '24 06:12 michaelchin

@michaelchin Okay, thank you!

Hojat-Shirmard avatar Dec 10 '24 06:12 Hojat-Shirmard