API Version 2.5

[cloneofghosts]

Describe the change

Pushes API V2.5.0 to the main branch. @alexander0042 the changes you made yesterday haven't been pushed to this repo.

EDIT: Dependabot needs to be excluded from the CLA agreement as its waiting for it to sign which it will never do.

Type of change

• Bugfixes to existing code
• New API Version
• Breaking change
• Code quality improvements to existing code or addition of tests
• Documentation Updates

Checklist

• This pull request fixes issue: fixes #
• Code builds locally. Your pull request won't be merged unless tests pass
• Code has been formatted using ruff
• I have signed the CLA agreement

[CLAassistant]

All committers have signed the CLA.

[cloneofghosts]

@alexander0042 Mentioned this in a discussion but you may have missed it: The development endpoint has crashed again and needs to be restarted. Would a restart policy help so it doesn't crash or would that not change anything?

You think this will launch next week or we looking more towards March before this can be launched? I know you're busy with the newborn but am curious as to when this may go live.

Also can #71 be merged or is there a reason why we're still using 5.9.0?

[alexander0042]

Ok, I'm back! Wild five weeks, but things have definitely settled down here! Starting with Pull #71, and then going from there

[cloneofghosts]

There's also now #73 but they're all minor updates so should be safe to update? I usually merge them in but I can stop doing that if you'd prefer to look at it yourself.

[alexander0042]

I tried to setup dependabot to update dockerfiles but the issue is they need to have dockerfile in the name otherwise it doesn't pick them up. I did some testing on my fork and was able to get dependabot to work for dockerfiles if I add "-dockerfile" to the end of the images.

I thought about updating the files in the repo but I realised that it might break your workflows so I left as is.

One quick thought on this: I really don't have any particular workflow with respect to these files. so totally ok to rename them!

[cloneofghosts]

I tried to setup dependabot to update dockerfiles but the issue is they need to have dockerfile in the name otherwise it doesn't pick them up. I did some testing on my fork and was able to get dependabot to work for dockerfiles if I add "-dockerfile" to the end of the images.
I thought about updating the files in the repo but I realised that it might break your workflows so I left as is.

One quick thought on this: I really don't have any particular workflow with respect to these files. so totally ok to rename them!

Added to my PR.

[alexander0042]

@alexander0042 Not sure why this came to my mind this morning but checking the Dark Sky docs for precipIntenisty it shows:

precipIntensity optional
The intensity (in inches of liquid water per hour) of precipitation occurring at the given time. This value is conditional on probability (that is, assuming any precipitation occurs at all).

See https://web.archive.org/web/20200331195132/https://darksky.net/dev/docs if you'd like to confirm for yourself. You can always add another field like snowIntensity which returns the non-liquid equivalent of snow and add the code in Pirate-Weather/pirateweather#89 for the precipIntensity field.

Would mean that the text thresholds for snow would need to be changed back to their liquid equivalent as the translation docs say the thresholds are based on that.

I've done some testing locally and using these values: -8.09 C, 8.37 m/s, 19.4293 mm I get 1.1347 as the liquid equivalent

UGH, great catch, and can't believe no one has noticed this over the past few years!

One the one hand, I really don't want to change it, since it would be a major breaking change to the API. On the other hand though, it would partially solve the "intensity during mixed snow/rain events" issue, and just generally be a cleaner way of doing things. On the balance though, I think we leave it as is, since it's been around for so long and probably more useful this way

[alexander0042]

There's also now #73 but they're all minor updates so should be safe to update? I usually merge them in but I can stop doing that if you'd prefer to look at it yourself.

Re: PR #73, this was the where I hit some issues with 2.5. Since it moved zarr over to V3, that actually is a decent change. Got it all working now though (it was just syntax), and planning on FINAILLY moving 2.5 to prod unless I'm missing something major

[cloneofghosts]

@alexander0042 Not sure why this came to my mind this morning but checking the Dark Sky docs for precipIntenisty it shows:

precipIntensity optional
The intensity (in inches of liquid water per hour) of precipitation occurring at the given time. This value is conditional on probability (that is, assuming any precipitation occurs at all).

See https://web.archive.org/web/20200331195132/https://darksky.net/dev/docs if you'd like to confirm for yourself. You can always add another field like snowIntensity which returns the non-liquid equivalent of snow and add the code in Pirate-Weather/pirateweather#89 for the precipIntensity field.
Would mean that the text thresholds for snow would need to be changed back to their liquid equivalent as the translation docs say the thresholds are based on that.
I've done some testing locally and using these values: -8.09 C, 8.37 m/s, 19.4293 mm I get 1.1347 as the liquid equivalent

UGH, great catch, and can't believe no one has noticed this over the past few years!

One the one hand, I really don't want to change it, since it would be a major breaking change to the API. On the other hand though, it would partially solve the "intensity during mixed snow/rain events" issue, and just generally be a cleaner way of doing things. On the balance though, I think we leave it as is, since it's been around for so long and probably more useful this way

Would splitting it into a snowfall and liquid intensity fix the issue? That way the precipIntensity works as Dark Sky did and the snowfall one would be as it is now or would that still be a big change?

[alexander0042]

@alexander0042 Not sure why this came to my mind this morning but checking the Dark Sky docs for precipIntenisty it shows:

precipIntensity optional
The intensity (in inches of liquid water per hour) of precipitation occurring at the given time. This value is conditional on probability (that is, assuming any precipitation occurs at all).

See https://web.archive.org/web/20200331195132/https://darksky.net/dev/docs if you'd like to confirm for yourself. You can always add another field like snowIntensity which returns the non-liquid equivalent of snow and add the code in Pirate-Weather/pirateweather#89 for the precipIntensity field.
Would mean that the text thresholds for snow would need to be changed back to their liquid equivalent as the translation docs say the thresholds are based on that.
I've done some testing locally and using these values: -8.09 C, 8.37 m/s, 19.4293 mm I get 1.1347 as the liquid equivalent

UGH, great catch, and can't believe no one has noticed this over the past few years!
One the one hand, I really don't want to change it, since it would be a major breaking change to the API. On the other hand though, it would partially solve the "intensity during mixed snow/rain events" issue, and just generally be a cleaner way of doing things. On the balance though, I think we leave it as is, since it's been around for so long and probably more useful this way

Would splitting it into a snowfall and liquid intensity fix the issue? That way the precipIntensity works as Dark Sky did and the snowfall one would be as it is now or would that still be a big change?

It would be an easy enough change, since we're already calculating them separately. However, what I'm more worried about is users who have set things up assuming precipitation intensity will show either snowfall or rain rates, and then suddenly all the snow amounts would be out of whack. I love the idea of adding another one in though, so we have additional snowIntensity and rainIntensity variables, much like accumulation

[cloneofghosts]

@alexander0042 The only thing I can see would be to change the docker image used in pirate-api. Currently its FROM nikolaik/python-nodejs:python3.12-nodejs22-slim and should just be a python image now that the node code is gone.

Ideally it should have the same python version as the ingest dockerfile. So should probably be python-3.13 since thats the latest version.

[cloneofghosts]

@alexander0042 Not sure why this came to my mind this morning but checking the Dark Sky docs for precipIntenisty it shows:

precipIntensity optional
The intensity (in inches of liquid water per hour) of precipitation occurring at the given time. This value is conditional on probability (that is, assuming any precipitation occurs at all).

See https://web.archive.org/web/20200331195132/https://darksky.net/dev/docs if you'd like to confirm for yourself. You can always add another field like snowIntensity which returns the non-liquid equivalent of snow and add the code in Pirate-Weather/pirateweather#89 for the precipIntensity field.
Would mean that the text thresholds for snow would need to be changed back to their liquid equivalent as the translation docs say the thresholds are based on that.
I've done some testing locally and using these values: -8.09 C, 8.37 m/s, 19.4293 mm I get 1.1347 as the liquid equivalent

UGH, great catch, and can't believe no one has noticed this over the past few years!
One the one hand, I really don't want to change it, since it would be a major breaking change to the API. On the other hand though, it would partially solve the "intensity during mixed snow/rain events" issue, and just generally be a cleaner way of doing things. On the balance though, I think we leave it as is, since it's been around for so long and probably more useful this way

Would splitting it into a snowfall and liquid intensity fix the issue? That way the precipIntensity works as Dark Sky did and the snowfall one would be as it is now or would that still be a big change?

It would be an easy enough change, since we're already calculating them separately. However, what I'm more worried about is users who have set things up assuming precipitation intensity will show either snowfall or rain rates, and then suddenly all the snow amounts would be out of whack. I love the idea of adding another one in though, so we have additional snowIntensity and rainIntensity variables, much like accumulation

The snowfall rates on the live API are already using the liquid representation of snowfall at the moment with the dev endpoint changing it back to the non-liquid version. Adding that code in wouldn't be a huge change to how it works currently on the live endpoint.

[alexander0042]

The snowfall rates on the live API are already using the liquid representation of snowfall at the moment with the dev endpoint changing it back to the non-liquid version. Adding that code in wouldn't be a huge change to how it works currently on the live endpoint.

Ok, that's a great point. It's been that way for a while now and no one complained, so that's a really good reason to keep it consistent! Let me see how hard it would be to switch it back/ add that variable in

[alexander0042]

Started adding those variables, but I realized there's not as much value as I though. Since minute will always have a type, all that variable would do is multiply the intensity by 10 if it's snowing, and 1 if it's raining. I know we did that with accumulations, but since they're used a fair bit more I think it's a little more justified. I left all the framework in the script though, so easy to change in the future

[cloneofghosts]

Started adding those variables, but I realized there's not as much value as I though. Since minute will always have a type, all that variable would do is multiply the intensity by 10 if it's snowing, and 1 if it's raining. I know we did that with accumulations, but since they're used a fair bit more I think it's a little more justified. I left all the framework in the script though, so easy to change in the future

It might be useful if/when this gets added in Pirate-Weather/pirateweather#89 since the snow to liquid ratio will change depending on temperature and wind speed.

If nothing else is being added to V2.5 then I think it should be good to go live and then #66 and any other issues you've missed can be added in V2.5.1 or V2.6.

[alexander0042]

Ok, added in! Also didn't have to chance the text generation, since accumulations are always used. The "currently" text does use intensity; however, so I added a factor to the conversion that's already being done!

[alexander0042]

Started adding those variables, but I realized there's not as much value as I though. Since minute will always have a type, all that variable would do is multiply the intensity by 10 if it's snowing, and 1 if it's raining. I know we did that with accumulations, but since they're used a fair bit more I think it's a little more justified. I left all the framework in the script though, so easy to change in the future

It might be useful if/when this gets added in Pirate-Weather/pirateweather#89 since the snow to liquid ratio will change depending on temperature and wind speed.

If nothing else is being added to V2.5 then I think it should be good to go live and then #66 and any other issues you've missed can be added in V2.5.1 or V2.6.

Oh yea, #66 definitely deserves a V2.6 title

[cloneofghosts]

Started adding those variables, but I realized there's not as much value as I though. Since minute will always have a type, all that variable would do is multiply the intensity by 10 if it's snowing, and 1 if it's raining. I know we did that with accumulations, but since they're used a fair bit more I think it's a little more justified. I left all the framework in the script though, so easy to change in the future

It might be useful if/when this gets added in Pirate-Weather/pirateweather#89 since the snow to liquid ratio will change depending on temperature and wind speed.
If nothing else is being added to V2.5 then I think it should be good to go live and then #66 and any other issues you've missed can be added in V2.5.1 or V2.6.

Oh yea, #66 definitely deserves a V2.6 title

Okay renamed. I had setup a simple version of the daily summaries in case its something you wanted to add in to V2.5 or a V2.5.x version but if you're moving it to V2.6 then the proper daily summaries should be used instead.

[cloneofghosts]

This has been merged up to the main branch. Once it gets pushed up to live the API docs will need to be updated but I think I covered off most things there already.