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

Move domain-specific functionality to subpackages #240

Merged
merged 39 commits into from
Jul 3, 2022
Merged

Conversation

lorenzoh
Copy link
Member

@lorenzoh lorenzoh commented Jun 20, 2022

This turns the domain-specific modules into packages as discussed in #186. Specifically:

  • FastAI.Vision -> FastVision
  • FastAI.Tabular -> FastTabular
  • FastAI.Textual -> FastText
  • FastMakie for Makie backend

This is obviously a gigantic breaking change and I'm thinking to tag this together with #196 to avoid subsequent breaking changes. Thoughts @darsnack? This should also be the last of the larger (and breaking) changes I have been planning to make, so if there are any other breakages planned, we should tag them together.

Closes #224, closes #241

Status:

  • create subpackages
  • move and fix tests
  • add test projects for subpackages
  • create CI for subpackages
  • apply JuliaFormatter (SciMLStyle)
  • update documentation to work with subpackages
    • update markdown sources and docstrings
    • rerun notebooks

@lorenzoh lorenzoh marked this pull request as draft June 20, 2022 11:33
@lorenzoh lorenzoh mentioned this pull request Jun 21, 2022
@lorenzoh lorenzoh marked this pull request as ready for review June 22, 2022 09:37
@darsnack
Copy link
Member

I think based on our experience with NNlib, the subpackage approach seems to be the best option moving forward. The only other possible breaking change I can think off is the data blocks API for time series support.

@darsnack
Copy link
Member

I tried doing a review, but I am confused about the final organization. Will a user load FastAI or FastVision? Right now, I don't see how FastAI requires FastVision, for example.

@lorenzoh
Copy link
Member Author

lorenzoh commented Jun 22, 2022

FastAI is now a core package, with FastVision etc. providing the domain-specifc functionality. So to run computer vision tasks, one imports using FastAI, FastVision. Loading the domain packages also populates the feature registries (datasets, datarecipes and learningtasks). With more domains being added, reducing the number of total packages that have to be loaded helps reduce the import time.

I am afraid the magnitude of changes and the automatic formatting will make a proper review hard, so feel free to focus on the big-picture changes

@lorenzoh
Copy link
Member Author

The only other possible breaking change I can think off is the data blocks API for time series support.

What would be breaking about that?

@darsnack
Copy link
Member

darsnack commented Jun 22, 2022

Maybe this has been resolved in more recent discussions. I recall that we mentioned how for sequential data, each sample is a sequence of features. So the outermost list is over samples like any dataset. But for Flux models, the outermost list is over time steps, and each element is a batch of features at a given time step. This means that there is an "encoding" (i.e. MLUtils.batchseq) that needs to happen after batching. It's definitely a solvable issue, but I didn't know how you wanted to handle it. Potentially the solution would be breaking or non-breaking.

@lorenzoh lorenzoh merged commit c0cbc53 into master Jul 3, 2022
lorenzoh added a commit that referenced this pull request Jul 5, 2022
@lorenzoh lorenzoh mentioned this pull request Sep 3, 2022
@lorenzoh lorenzoh deleted the lo/fastvision branch October 21, 2022 17:45
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.

Make a subpackage for Makie support Makie 0.17 support
2 participants