PB-1169 Search forecast properties #500
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asteiner-swisstopo
approved these changes
Jan 23, 2025
Comment on lines
+112
to
+113
| queryset: | ||
| A django queryset (https://docs.djangoproject.com/en/3.0/ref/models/querysets/) |
There was a problem hiding this comment.
That refers to self, right? I would not list that here.
Same on line 131.
| queryset: | ||
| A django queryset (https://docs.djangoproject.com/en/3.0/ref/models/querysets/) | ||
| date_time: | ||
| A string |
There was a problem hiding this comment.
Maybe add an example of how this would typically look like ('2020-10-28T13:05:10Z').
| queryset: | ||
| A django queryset (https://docs.djangoproject.com/en/3.0/ref/models/querysets/) | ||
| start_datetime: | ||
| A string with the start datetime |
There was a problem hiding this comment.
I would add a note that this can also be ".."
Comment on lines
+331
to
+342
| def filter_by_forecast_horizon(self, duration): | ||
| return self.get_queryset().filter_by_forecast_horizon(duration) | ||
|
|
||
| def filter_by_forecast_duration(self, duration): | ||
| return self.get_queryset().filter_by_forecast_duration(duration) | ||
|
|
||
| def filter_by_forecast_variable(self, val): | ||
| return self.get_queryset().filter_by_forecast_variable(val) | ||
|
|
||
| def filter_by_forecast_perturbed(self, pert): | ||
| return self.get_queryset().filter_by_forecast_perturbed(pert) | ||
|
|
There was a problem hiding this comment.
Couldn't all these be replaced by something less redundant?
For example
def filter_by_field(self, field_name, value):
return self.get_queryset().filter(**{field_name: value})
Comment on lines
+591
to
+602
| cls.items = cls.factory.create_item_samples( | ||
| [ | ||
| 'item-forecast-1', | ||
| 'item-forecast-2', | ||
| 'item-forecast-3', | ||
| 'item-forecast-4', | ||
| 'item-forecast-5' | ||
| ], | ||
| cls.collection, | ||
| db_create=True, | ||
| ) |
There was a problem hiding this comment.
Consider creating these items individually with create_item_sample such that you can set the name of the samples. Otherwise in the tests you refer to the automatically set names ("item-1", "item-2", ...) instead of the ones on the item ("item-forecast-1", "item-forecast-2", ...).
Add forecast properties to search filter for GET and POST. Refactor forecast:variable and forecast:perturbed descriptions into schema properties to be reused.
Add filters for forecast properties to search endpoints. Can filter for forecast_reference_datetime either by exact value or by a range.
Decided to remove the forecast parameters because the variable name differs to the post variable names (using underscore instead of colon) and colons need url encoding.
Decided to remove the forecast filtering options from the GET /search request due to colons in the parameter names. Also fixed the POST request body to actually use the parameters that contain a colon.
benschs
force-pushed
the
feat-pb-1169-search-forecast
branch
from
8c248f9 to
3e0d8fc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
spec
Add forecast properties to search filter for GET and POST. Refactor
forecast:variableandforecast:perturbeddescriptions into schema properties tobe reused.
New spec for Get /search
New spec for POST /search
In the GET parameters I decided to use underscores(_) instead of colons(:) for the parameter names as colons need to be url encoded. The parameter names are quite long, but I think it's ok to be more descriptive, otherwise we could also shorten forecast to fc (e.g.
fc_reference_datetime). In the POST parameters i kept the colon in the names to stay consistent.Implementation
Add filters for forecast properties to search endpoints.
Can filter for
forecast_reference_datetimeeither by exact value or by a range.The filter for
forecast_perturbedis a boolean, but in the GET parameters python does not accept the valuesfalse/true, only capitalizedFalse/Trueare accepted. Do we have a convention on how to handle this?I did not add any indexes for the forecast fields. Do you think we should add them already on all fields? I can't really tell how the search will be used and if it makes sense to just add an index as it will make writes slower.