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

When a user specifies that they want unit fields, return is empty. #184

Open
cassienickles opened this issue May 21, 2024 · 5 comments
Open
Assignees
Labels
bug Something isn't working

Comments

@cassienickles
Copy link
Member

When a user specifies fields = "reach_id,time,time_units,time_tai,time_tai_units,time_str,p_lat,wse" it returns empty. Upon investigation, it only returns empty when the unit fields are added (time_units, time_tai_units, etc). If those fields are left out from being specified, hydrocron works as expected and the units fields are appended in the results. (ie. fields = "reach_id,time,time_tai,time_str,p_lat,wse" ) Is this an error in the code somewhere?

@torimcd
Copy link
Collaborator

torimcd commented May 22, 2024

The error that gets returned is "400: fields parameter should contain valid SWOT fields"

Looks like the code that adds units may be adding "_units" again when the units are in the requested fields, eg resulting in invalid fields like time_units_units:
https://github.com/podaac/hydrocron/blob/ab4ffb74389c37fbf99e4beb6c8493847ffa3a4d/hydrocron/api/controllers/timeseries.py#L329C1-L340C34

Might need to add a check to skip this for fields where units are already specified, but this gets complicated in the example above when some units fields are explicitly requested and other fields that have units are not, like wse.

@cassienickles what behavior do you expect in the example you posted? Would you expect the units to also be returned for the wse field even though it's not explicitly requested?

@torimcd torimcd added the bug Something isn't working label May 22, 2024
@torimcd torimcd moved this to 📋 Backlog in SOTO PI 24.2 May 22, 2024
@cassienickles
Copy link
Member Author

I'm wondering that since we automatically add the units if a parameter has units, maybe we don't make it known in the documentation that the '_units' fields can be requested and instead let them know that the '_units' fields will just automatically be returned if they are available.

So in this documentation (https://podaac.github.io/hydrocron/timeseries.html#request-parameters), remove the '_units' parameters from fields that can be requested and add a blurb that unit fields will be returned as well? Thoughts?

@nikki-t
Copy link
Collaborator

nikki-t commented May 24, 2024

What I think is happening is the API code will look at the constants.REACH_ALL_COLUMNS and constants.NODE_ALL_COLUMNS to determine if the user passed in a valid list of fields in their request. See code here.

Since the units are not specified in the reach and node column lists, the API code returns an invalid request response.

The way that the API code is written now, is if we add units to those lists then the API code will try to add units to the units fields as mentioned by @torimcd.

@cassienickles - I like your idea of removing the _unit fields from the documentation since we always return the units if they are available and we can avoid complicated conditional statements to try to accommodate various requests for different units. Happy to change things in the future if the need arises!

For now, I will update the documentation since I also need to spend some time with the documentation in issue #183.

@nikki-t nikki-t moved this from 📋 Backlog to 🏗 In progress in SOTO PI 24.2 May 24, 2024
@nikki-t nikki-t mentioned this issue May 24, 2024
4 tasks
@nikki-t nikki-t moved this from 🏗 In progress to 👀 In review in SOTO PI 24.2 May 28, 2024
@torimcd
Copy link
Collaborator

torimcd commented May 30, 2024

I think updating the documentation is fine for the immediate issue, but I think we should be able to have the API handle this use case gracefully in the future. We want people to be using units, so once they see them being returned it makes sense that they will request them in future API calls.

Ideally, we would also have the units listed in the fields that can be returned. We should think about how to do this in line with #112 as well

@frankinspace
Copy link
Member

👍 handle it in documentation for now

@nikki-t nikki-t moved this from 👀 In review to ✅ Done in SOTO PI 24.2 Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: ✅ Done
Development

No branches or pull requests

4 participants