Skip to content

Refactors Label Times #135

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

Merged
merged 53 commits into from
Jun 17, 2020
Merged

Refactors Label Times #135

merged 53 commits into from
Jun 17, 2020

Conversation

jeff-hernandez
Copy link
Contributor

@jeff-hernandez jeff-hernandez commented May 20, 2020

The label times object was refactored by:

  • isolating deserialization functions and label descriptions into separate files
  • improving checks and inference for target name and type
  • simplifying docstring tests for bin transform

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #135 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #135   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        26    +4     
  Lines          967       960    -7     
=========================================
- Hits           967       960    -7     
Impacted Files Coverage Δ
composeml/label_times/plots.py 100.00% <ø> (ø)
composeml/__init__.py 100.00% <100.00%> (ø)
composeml/conftest.py 100.00% <100.00%> (ø)
composeml/label_maker.py 100.00% <100.00%> (ø)
composeml/label_times/__init__.py 100.00% <100.00%> (ø)
composeml/label_times/description.py 100.00% <100.00%> (ø)
composeml/label_times/deserialize.py 100.00% <100.00%> (ø)
composeml/label_times/object.py 100.00% <100.00%> (ø)
composeml/tests/test_label_serialization.py 100.00% <100.00%> (ø)
composeml/tests/test_label_times.py 100.00% <100.00%> (ø)
... and 6 more

@jeff-hernandez jeff-hernandez requested a review from rwedge May 22, 2020 20:07
@jeff-hernandez jeff-hernandez linked an issue May 29, 2020 that may be closed by this pull request
@jeff-hernandez jeff-hernandez requested a review from rwedge June 3, 2020 15:43
@jeff-hernandez jeff-hernandez requested a review from rwedge June 3, 2020 20:00
Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the changes. Do we want to add a warning based on if the schema version is older / newer than expected?

@jeff-hernandez
Copy link
Contributor Author

Yes, I think that's a good idea @rwedge. We can add a warning as a result from different schema versions. Perhaps it would be easier to do this in a separate PR.

@rwedge
Copy link
Contributor

rwedge commented Jun 16, 2020

Yes, I think that's a good idea @rwedge. We can add a warning as a result from different schema versions. Perhaps it would be easier to do this in a separate PR.

Sounds good, can you make an issue for it?

@jeff-hernandez
Copy link
Contributor Author

Yes, tracking this issue at #145.

Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@jeff-hernandez jeff-hernandez merged commit e6325dd into master Jun 17, 2020
@jeff-hernandez jeff-hernandez deleted the label_times_refactor branch June 26, 2020 19:50
@jeff-hernandez jeff-hernandez mentioned this pull request Jul 2, 2020
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

Successfully merging this pull request may close these issues.

Update default column name used for cutoff time column
2 participants