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.

Sign up for GitHub

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

Jump to bottom

set period if dimension cyclicity automatically set #237

Closed
ThibHlln opened this issue Jul 19, 2021 · 4 comments · Fixed by #267
Closed

set period if dimension cyclicity automatically set #237

ThibHlln opened this issue Jul 19, 2021 · 4 comments · Fixed by #267
Labels
bug Something isn't working

Comments

@ThibHlln
Copy link

ThibHlln commented Jul 19, 2021 •
edited
Loading

Hi Sadie, Hi David,

I've noticed that when I define a field with a cyclic spatial dimension (e.g. -180 degrees east to +180 degrees east), I get an error message in cf-python==3.10.0 that I was not getting in cf-python==3.8.0 because my cyclic dimension does not have a period. See example below:

import cf

f = cf.Field()

lon = f.set_construct(
    cf.DimensionCoordinate(
        properties={'standard_name': 'longitude',
                    'units': 'degrees_east',
                    'axis': 'X'},
        data=cf.Data([-150, -90, -30, 30, 90, 150]),
        bounds=cf.Bounds(data=cf.Data([[-180, -120], [-120, -60], [-60, 0], [0, 60], [60, 120], [120, 180]]))
    ),
    axes=f.set_construct(cf.DomainAxis(size=6))
)

With this example, when setting the longitude construct, I get thrown the following:

Traceback (most recent call last):
File "", line 1, in
File "/Users/thibhlln/opt/miniconda3/envs/hj38/lib/python3.8/site-packages/cf/mixin/fielddomain.py", line 2651, in set_construct
self.autocyclic(key=out, coord=construct, config=autocyclic)
File "/Users/thibhlln/opt/miniconda3/envs/hj38/lib/python3.8/site-packages/cfdm/decorators.py", line 181, in verbose_override_wrapper
return method_with_verbose_kwarg(*args, **kwargs)
File "/Users/thibhlln/opt/miniconda3/envs/hj38/lib/python3.8/site-packages/cf/mixin/fielddomain.py", line 1244, in autocyclic
self.cyclic(key, iscyclic=True, period=period, config=config)
File "/Users/thibhlln/opt/miniconda3/envs/hj38/lib/python3.8/site-packages/cf/field.py", line 5116, in cyclic
raise ValueError(
ValueError: A cyclic dimension coordinate must have a period

I don't know if cyclicity was automatically checked for and set before, and I believe that this is a good thing that it is if the bounds show that it is a cyclic dimension, however, I am wondering whether the period could not also be set automatically at the same time? This would be very convenient.

Thank you.
Thibault
@ThibHlln ThibHlln added the enhancement New feature or request label Jul 19, 2021
@sadielbartholomew
Copy link
Member

sadielbartholomew commented Jul 19, 2021 •
edited
Loading

Thanks for raising this Thibault. David would likely know right away when and why the underlying change leading to this behaviour change was made, but he is on leave this week, so I have done a little digging to investigate.

Origin of change

There is nothing obvious in the changelog to indicate why there is the behaviour change (you may have already checked that) but from a git blame on the ultimate line referenced in the traceback and it appears that the change was made as part of (or, at least, included in with) a widespread set of API changes towards better performance, made in the PR #210.

Specifically, amongst other updates, the following lines were incorporated and it seems like there is a mistake here (that I must have missed in reviewing the PR), because any existing period is converted to None and I can't see why that is logical in context:

cf-python/cf/mixin/fielddomain.py

Lines 1168 to 1173 in fa951fe

else:
period = coord.period()
if period is not None:
period = None
else:
period = config.get("period")

So I will look into that as a possible bug.

Feature request

I am wondering whether the period could not also be set automatically at the same time? This would be very convenient.

If there is a small bug as the above may suggest, then hopefully when fixed the period will be set automatically as you desire. If not, I can't see any reason we can't adapt the code to do it, especially if it would be convenient to you and likely others.

So, I'll get onto it later today or tomorrow and report back when I have determined the exact cause and way to add your feature request. Thanks.

Do you have a workaround for now? I guess you can set the period manually afterwards?

@ThibHlln
Copy link
Author

ThibHlln commented Jul 19, 2021 •
edited
Loading

So, I'll get onto it later today or tomorrow and report back when I have determined the exact cause and way to add your feature request. Thanks.

Thank you Sadie. I registered this feature request (which may just be a bug actually) before forgetting about it, but there is no urgency for it to be solved today (or even this week), so please feel free to wait a little longer, e.g. until David returns.

Do you have a workaround for now? I guess you can set the period manually afterwards?

I don't know if there is a workaround, as I think the autocyclic method is called when I am setting the construct, and I don't think you can manually set cyclicity+period at the time of setting the construct. At least, I am not aware of any way of achieving this, so the construct setting fails before I get a chance to do f.dim('X').period(...).

@sadielbartholomew sadielbartholomew added the question General question label Jul 20, 2021
@sadielbartholomew
Copy link
Member

Thanks for the extra info., Thibault. Since you don't consider it urgent then I think it is probably best to wait to hear from David because, since he made the changes in question that changed the behaviour, I suspect it will take him very little time to pinpoint what caused it, whether it is a bug, and how we can incorporate your request, whereas it will take me O(10) as much time, at least, to work it out. All for efficiency!

@davidhassell (when you are back from leave, of course) please let us know your thoughts (the thread here probably covers everything you need to know)...

@davidhassell davidhassell added bug Something isn't working and removed enhancement New feature or request question General question labels Sep 30, 2021
@davidhassell davidhassell mentioned this issue Sep 30, 2021
Merged
davidhassell added a commit to davidhassell/cf-python that referenced this issue Sep 30, 2021
@davidhassell
NCAS-CMS#237
3c727b4
davidhassell added a commit to davidhassell/cf-python that referenced this issue Sep 30, 2021
@davidhassell
NCAS-CMS#237
d1e0119
@ThibHlln
Copy link
Author

Awesome. Thank you David and Sadie! 🚀

ThibHlln pushed a commit to ThibHlln/unifhy that referenced this issue Oct 20, 2021
change dependency version for cf-python
c0c1b82 
This version includes:
- fix for memory leak due to ESMF objects NCAS-CMS/cf-python#225
- lift for size 1 restriction on grid NCAS-CMS/cf-python#249
- fix bug preventing cyclic domain (e.g. global runs) NCAS-CMS/cf-python#237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix autocyclic bug davidhassell/cf-python
3 participants
@davidhassell @ThibHlln @sadielbartholomew