-
Notifications
You must be signed in to change notification settings - Fork 1
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
Handle climo files with separate chronology #60
Conversation
Keep alternative versions in case ...
And clean up populateddb fixture
i.e. tests based on populateddb fixture. Goal is to remove non standards compliant test files cgcm.nc an cgcm-tmin.nc.
.filter(TimeSet.time_resolution == timescale) | ||
|
||
.filter(DataFileVariable.ensembles.any(Ensemble.name == ensemble_name)) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grouped related joins and filters, but this may not seem to others, as it does to me, a clearer way to organize the query.
30f2b2c
to
a1b862c
Compare
Maybe I'm misunderstanding how this query works now, but I think this docstring in |
('seasonal', 2, (1985, 7, 15)), | ||
('yearly', 0, (1985, 7, 2)), | ||
)) | ||
def test_data_single_file(populateddb, variable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we do any exception testing here? E.g. what happens if you give time_idx=1
for timescale='yearly'
, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what happens at present.
The frontend data validation functions check to see (line 93) if the backend returns a string object instead of a JSON object to catch that sort of thing.
There may be a better way to deal with this, but that's what happens now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, yeah we definitely want to avoid the backend returning 500s!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #57
The fix itself was simple. Modifying the tests to replace completely non standards-compliant test files
cgcm.nc
andcgcm-tmin.nc
with standards compliant ones took most of the work.