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

Reconstruct with all fieldnames #7

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

ToucheSir
Copy link
Member

This ensures that important, non-fmappable properties with non-default values are preserved. Should address #6 and #3 (comment).

I believe this is theoretically breaking, but a search on JuliaHub doesn't turn up any uses of the macro that specify a list of fields.

This ensures that important, non-fmappable properties with non-default values are preserved.
@devmotion
Copy link

I guess the sentence "For a discussion regarding implementing functors for which only a subset of the fields are "seen" by functor, see here." could be removed from the README and replaced with an example.

@DhairyaLGandhi
Copy link
Member

Seems sensible at a first glance. Thanks!

@ToucheSir
Copy link
Member Author

Updated docs per @devmotion's comment. @DhairyaLGandhi can we cut a release with this so that Flux can start using it? I've seen a fair few PRs lately that stand to benefit.

@CarloLucibello
Copy link
Member

@DhairyaLGandhi bump

@DhairyaLGandhi
Copy link
Member

Seems alright. This would make a breaking release though.

@DhairyaLGandhi DhairyaLGandhi merged commit 43ae832 into FluxML:master Jan 20, 2021
@CarloLucibello
Copy link
Member

tag?

@ToucheSir
Copy link
Member Author

Yup, I was thinking a 0.2 so as to make it in time for Flux 0.12 (or barring that, 0.13)

@DhairyaLGandhi
Copy link
Member

Cool, good that we're in agreement then

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.

4 participants