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

Add abstractions to make it easier to declare and instantiate models #605

Merged
merged 22 commits into from
Jan 22, 2023

Conversation

@NickleDave NickleDave marked this pull request as draft December 25, 2022 02:31
@NickleDave
Copy link
Collaborator Author

Still need to add tests for new classes, but current tests pass

@NickleDave
Copy link
Collaborator Author

NickleDave commented Dec 25, 2022

This should probably also include a very initial version of #37 that at least documents how these abstractions work, taken from my dev notes

  • vignette: adding a WindowedFrameClassificationModel
  • reference: abstractions for models, base class + decorator; goals and design decisions

@NickleDave NickleDave force-pushed the make-easier-instantiate-model branch 3 times, most recently from fa70f59 to 75f3f39 Compare January 17, 2023 03:07
@NickleDave NickleDave mentioned this pull request Jan 17, 2023
44 tasks
@NickleDave NickleDave force-pushed the make-easier-instantiate-model branch 5 times, most recently from 1124b77 to 2994eb3 Compare January 21, 2023 21:48
@NickleDave NickleDave marked this pull request as ready for review January 21, 2023 22:24
@NickleDave
Copy link
Collaborator Author

NickleDave commented Jan 21, 2023

@yardencsGitHub tests for this now pass locally 😩

I found a way to make the abstractions a little more succinct than when we talked through it. Basically, there's just a single decorator @model that just modifies a ModelDefinition in place to convert it into a model class that sub-classes a model family.

Assuming tests pass on CI, I will go ahead and merge in.

In the future let's do more async review, but I would really like to make progress on version 1.0 while I have a little time to focus on it.

@NickleDave
Copy link
Collaborator Author

I am not understanding why the CI is failing.

I think it may be related to this?
actions/runner-images#6931

I also thought it might be an old version of generated test data on the OSF repo but I tried downloading the generated data for ci locally and then running the tests the same way we do on CI, but this did not reproduce the error--the tests are running fine without failure

It's also weird we get specific test failures but no error messages at the end:

2023-01-22T00:58:43.9564606Z nox > pytest --models teenytweetynet --cov=./ --cov-report=xml
2023-01-22T00:58:55.3302457Z ============================= test session starts ==============================
2023-01-22T00:58:55.3303545Z platform linux -- Python 3.9.16, pytest-7.2.1, pluggy-1.0.0
...
2023-01-22T01:14:45.8300540Z tests/test_core/test_predict.py FFFF.....   
...
2023-01-22T01:52:59.9927859Z tests/test_models/test_base.py ......................FF.....             [ 91%]
2023-01-22T01:53:00.0228373Z tests/test_models/test_decorator.py ..............                       [ 92%]
2023-01-22T01:53:00.0666356Z tests/test_models/test_definition.py ....................                [ 93%]
2023-01-22T01:53:00.3391652Z tests/test_models/test_teenytweetynet.py ..............                  [ 94%]
2023-01-22T01:53:00.3856049Z tests/test_models/test_tweetynet.py ..s                                  [ 94%]
2023-01-22T01:53:00.8246889Z tests/test_models/test_windowed_frame_classification_model.py .......... [ 95%]
2023-01-22T01:53:01.9587868Z ...............F  
...

Just going to go ahead and merge

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.

None yet

1 participant