Skip to content

Conversation

@d-netto
Copy link
Contributor

@d-netto d-netto commented Jul 6, 2020

No description provided.

@andreasnoack
Copy link
Contributor

We used to have a version like that but the result can't be inferred so it's too costly.

@ChrisRackauckas
Copy link
Member

It needs to be done since Zygote cannot infer through the constructor with this mutation. Maybe the easiest fix for now would be to create new dispatches so that Missing does what it currently does and a non-missing dispatch that just does the no-mutating thing but fast. That will make everything with Zygote work well while not impacting Pumas speeds.

@andreasnoack
Copy link
Contributor

It’s fine to rewrite these methods such that they work with Zygote. We just shouldn’t use DataFrames for it because of the inference issue.

@d-netto
Copy link
Contributor Author

d-netto commented Jul 8, 2020

I believe the latest commit whould be a possible solution without using dataframes.

Copy link
Member

@ChrisRackauckas ChrisRackauckas left a comment

Choose a reason for hiding this comment

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

Yeah, that should be good. I think you can get rid of the collect, but I know Iterators.filter is quite slow so maybe it's better to collect for now anyways. We can go with this and always improve if it's an issue. Let's get an issue open about this minor performance detail and move on.

@d-netto
Copy link
Contributor Author

d-netto commented Jul 8, 2020

Ok, it's passing.

@ChrisRackauckas ChrisRackauckas merged commit ec1bbb2 into SciML:master Jul 8, 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.

3 participants