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.

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

New NRFA API #17

Closed
cvitolo opened this issue Apr 13, 2019 · 6 comments
Closed

New NRFA API #17

cvitolo opened this issue Apr 13, 2019 · 6 comments

Comments

@cvitolo
Copy link
Owner

cvitolo commented Apr 13, 2019

The package needs to be updated according to the new API.

The link to the API documentation is:
https://nrfaapps.ceh.ac.uk/nrfa/nrfa-api.html

cvitolo added a commit that referenced this issue Apr 14, 2019
@cvitolo
Copy link
Owner Author

cvitolo commented Apr 14, 2019

A basic modification of the package based on the new API is now implemented.

@cvitolo cvitolo closed this as completed Apr 14, 2019
@cvitolo cvitolo reopened this Apr 14, 2019
@ilapros
Copy link
Collaborator

ilapros commented Jun 27, 2019

As I had discussed with Claudia a while ago I'd be happy to help out with implementing ways to use the new NRFA api in rnrfa.
@mattfry-ceh: any idea of what might change in the future in the API
@griffada: still interested in looking into this?

@mattfry-ceh
Copy link

Hi. I had emailed @cvitolo to verify whether having hyphen's in the parameter names was a big problem. To cut a long story short: I appreciate it's a bit annoying for R users but it seems there are a number of ways around it. The RNRFA package would be a great way to "shield" newby R developers from issues of having to access APIs anyway. We would really love to get the RNRFA package to use the new API. Some comments:

  • We don't have plans to remove the ad hoc services the package used before for now - they are still used by some bits of the NRFA website
  • Following the limited feedback we got, we think we will stick with the API as it is
  • Future changes would predominantly be adding things (there's a new FEH catchment descriptor coming out, then new LCM2015 statistics, and data types including one that merges NRFA data with live data from the EA). So no changes to the structure, just additions.

Let me know if you would like me to do anything.

We'd also like to develop some simple guidance, but the first point for R users would be to use RNRFA.

@cvitolo
Copy link
Owner Author

cvitolo commented Jul 25, 2019

@mattfry-ceh thanks for the comment. The rnrfa package already uses the new API and the hyphens were not a problem in the end.

@ilapros I agree there are a number of improvements we could make and it would be good to plan a major update.

@ilapros
Copy link
Collaborator

ilapros commented Jul 30, 2019

@cvitolo what improvements were you thinking about? I have played a bit with the new functions and they seem to work well, maybe we could document the new data types available but overall your wrappers for the new API seem to do what one would want. I would maybe have these changes

  • output time-series tibbles have the date processed as a date rather than left as a rowname
  • polish the column-names of catalogue()
    but these are simple things and I'd be and happy to give a first try to implement them.

I have one major issue in the amax-flow/stage and pot-flow/stage types, which is not related to the rnrfa package but to the NRFA API, I have tried to start a conversation about the issue at #19. This could be fixed by changing what the NRFA API outputs, but probably it is easier to resolve on our end.

@cvitolo
Copy link
Owner Author

cvitolo commented Aug 11, 2019

Hi @ilapros , it would be great if you could implemented the two points above. Thank you!

We could certainly create a workaround to issue #19 but I would consult with @mattfry-ceh to see if the issue can be resolved on the NRFA API side.

@cvitolo cvitolo closed this as completed Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants