Remove default binning #874
Merged
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
paolafer
force-pushed
the
remove-default-binning
branch
from
2511e12 to
67cfb9e
Compare
|
Just so we don't forget, there is still a problem if the first file does not contain sensor responses. If we don't fix it here, we will open an issue. |
paolafer
force-pushed
the
remove-default-binning
branch
from
3b748dd to
35e57d4
Compare
paolafer
force-pushed
the
remove-default-binning
branch
3 times, most recently
from
8e58723 to
34fa42a
Compare
paolafer
force-pushed
the
remove-default-binning
branch
from
34fa42a to
c8e7fec
Compare
|
I've rebased on the new master. The PR is now ready to be reviewed one last time. |
paolafer
force-pushed
the
remove-default-binning
branch
from
c8e7fec to
e9410d9
Compare
gonzaponte
reviewed
Jun 13, 2024
paolafer
force-pushed
the
remove-default-binning
branch
from
e9410d9 to
23ca5ef
Compare
gonzaponte
approved these changes
Jun 14, 2024
There was a problem hiding this comment.
Excellent. We remove one of the many instances of dangerous default values in our code. We now handle the case with missing values explicitly, but we still have to address the case where the first file does not contain data, for which we will open an issue. The code looks good and is properly tested. Good job!
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.
This PR removes the assignment of a default time binning to sensors, when no sensor response is found in the bunch of files being processed. The city will crash, instead.
Also, a test is modified to account for this change and another one is added.
Closes #857