set period if dimension cyclicity automatically set

[ThibHlln]

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

[sadielbartholomew]

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]

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]

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)...

[ThibHlln]

Awesome. Thank you David and Sadie! 🚀