Updating Firefly-dependent areas to match API changes and PyPI packaging #4143

mtryan83 · 2022-09-24T21:36:25Z

PR Summary

This PR aims to reconcile the changes to Firefly introduced since version 4.0.0 of yt was released. In summary, Firefly tweaked the Reader and ParticleGroup APIs and the installation of Firefly from PyPI installs as firefly (instead of Firefly or Firefly-vis, the devs were able to get control of the package name). I've updated the documentation, including that (I believe) Firefly also now prefers their binary .ffly format.

I'm very new to Firefly and yt, so I would greatly appreciate it if e.g. @agurvich would double check.

I believe this counts as a fix/update for #3415

PR Checklist

New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

Current version of Firefly is 3.2.0. These seem to be the only changes necessary to convert to the new python API. (API changes seem to mostly be a relabeling of parameter fields, plus velocities is elevated to a separate field. )

When installed from PyPI (e.g. using pip install firefly), the firefly module is brought in as lower-case.

welcome · 2022-09-24T21:36:27Z

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

Removed excess whitespace to make flake8 happy.

mtryan83 · 2022-09-24T21:54:13Z

pre-commit.ci autofix

neutrinoceros

Hi @mtryan83, thanks a lot for doing this !
Indeed, it'd be great if @agurvich could sign off here.
In the mean time, here are a couple comments, one of which is crucial (API change)

setup.cfg

yt/data_objects/data_containers.py

yt/utilities/on_demand_imports.py

Co-authored-by: Clément Robert <cr52@protonmail.com>

…firefly

Followed approach used in yt/data_objects/image_array, since as far as I can tell, datadir is just the new name of JSONdir (because data files are now generally .ffly instead of .json).

Fixes to the actual function call documentation that I missed earlier.

neutrinoceros

follow up comments on the deprecated API part

yt/data_objects/data_containers.py

Co-authored-by: Clément Robert <cr52@protonmail.com>

yt/data_objects/data_containers.py

Additional kwargs passed to create_firefly_object are passed unchanged to the firefly.data_reader.Reader initialization.

yt/data_objects/data_containers.py

Co-authored-by: Clément Robert <cr52@protonmail.com>

agurvich · 2022-09-26T21:29:18Z

So, I am only just now tuning in. I am traveling until next Monday 10/3 and won't have a chance to formally review this until then. @mtryan83, first of all, thank you for putting all of this together! from your original PR summary this seems like exactly the sort of changes that I've had on a to-do list to do since we updated the API and submitted the Firefly paper so I am in your debt for handling it.

It's been hard to follow the updates to the PR since then, has it all mostly been yt documentation stuff? Is there something in particular I should pay close attention to (it sounds like you covered all the bookkeeping stuff like switching JSONDIR and .ffly format, etc.). I can have a look at the file diffs but it may be worth coordinating and changing things upstream if there's something worth cleaning up there (e.g. if it means removing a warning on yt's side because I just do better type checking on firefly's side).

neutrinoceros · 2022-09-27T12:31:21Z

So, I am only just now tuning in. I am traveling until next Monday 10/3 and won't have a chance to formally review this until then.

No rush on our side !

it may be worth coordinating and changing things upstream if there's something worth cleaning up there (e.g. if it means removing a warning on yt's side because I just do better type checking on firefly's side).

AFAICT there's nothing we'd want to clean up upstream.

mtryan83 · 2022-09-27T17:45:21Z

I think the main thing is just making sure I updated the API correctly. From what I could tell, it was just JSONdir --> datadir, tracked_* --> field_*, Firefly --> firefly, and dumpToJSON --> writeToDisk and options --> settings (though those two only appear in the documentation).

I don't see anything to clean up upstream either.

Looks like I missed a JSONdir though, @neutrinoceros, sorry!

neutrinoceros · 2022-09-28T18:54:05Z

For later reference, I previously triaged this for the 4.1 milestone, but I'm going to take it back, because that release is already far behind schedule. I would still be happy to include the patch in the release if we can get a second review in time, which seems likely, I just want to signal that the patch shouldn't be considered blocking.

# Conflicts: # setup.cfg

mtryan83 · 2022-10-03T20:55:04Z

(fast-cache and updated firefly dependencies were touching! Couldn't automerge anymore)

agurvich

These look great to me, thank you @mtryan83 for cleaning all this up with the new API. I'm hoping that I won't be compelled to change it again any time soon now that we have the .ffly format (which is what inspired most of the changes).

welcome · 2022-10-03T21:47:32Z

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆

mtryan83 and others added 3 commits September 22, 2022 11:36

Firefly API update

230cf53

Current version of Firefly is 3.2.0. These seem to be the only changes necessary to convert to the new python API. (API changes seem to mostly be a relabeling of parameter fields, plus velocities is elevated to a separate field. )

Import fix

4395776

When installed from PyPI (e.g. using pip install firefly), the firefly module is brought in as lower-case.

Merge branch 'yt-project:main' into main

caacf38

mtryan83 changed the title ~~Updating Firefly-dependent areas to match API changes and PyPI packaging~~ Bug: Updating Firefly-dependent areas to match API changes and PyPI packaging Sep 24, 2022

mtryan83 changed the title ~~Bug: Updating Firefly-dependent areas to match API changes and PyPI packaging~~ Updating Firefly-dependent areas to match API changes and PyPI packaging Sep 24, 2022

Fixed some whitespace issues

a8b7ed7

Removed excess whitespace to make flake8 happy.

neutrinoceros added the enhancement Making something better label Sep 25, 2022

neutrinoceros requested changes Sep 25, 2022

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

yt/data_objects/data_containers.py Outdated Show resolved Hide resolved

yt/utilities/on_demand_imports.py Outdated Show resolved Hide resolved

mtryan83 and others added 5 commits September 25, 2022 14:03

Update setup.cfg

3d2b34d

Co-authored-by: Clément Robert <cr52@protonmail.com>

Updating class name to keep in sync

a455282

Merge remote-tracking branch 'origin/updating_firefly' into updating_…

b53b0d6

…firefly

Re-added JSONdir argument with deprecation warning

12c54cc

Followed approach used in yt/data_objects/image_array, since as far as I can tell, datadir is just the new name of JSONdir (because data files are now generally .ffly instead of .json).

Documentation fixes

a4beb1f

Fixes to the actual function call documentation that I missed earlier.

neutrinoceros reviewed Sep 25, 2022

View reviewed changes

yt/data_objects/data_containers.py Show resolved Hide resolved

yt/data_objects/data_containers.py Show resolved Hide resolved

yt/data_objects/data_containers.py Outdated Show resolved Hide resolved

mtryan83 and others added 2 commits September 25, 2022 15:13

Update yt/data_objects/data_containers.py

2319e29

Co-authored-by: Clément Robert <cr52@protonmail.com>

Update yt/data_objects/data_containers.py

39885ce

Co-authored-by: Clément Robert <cr52@protonmail.com>

mtryan83 commented Sep 25, 2022

View reviewed changes

yt/data_objects/data_containers.py Outdated Show resolved Hide resolved

mtryan83 and others added 2 commits September 25, 2022 16:32

Update yt/data_objects/data_containers.py

eae6f97

Additional kwargs documentation

0302ffc

Additional kwargs passed to create_firefly_object are passed unchanged to the firefly.data_reader.Reader initialization.

neutrinoceros reviewed Sep 26, 2022

View reviewed changes

yt/data_objects/data_containers.py Outdated Show resolved Hide resolved

neutrinoceros added this to the 4.1.0 milestone Sep 26, 2022

Update yt/data_objects/data_containers.py

4f61199

Co-authored-by: Clément Robert <cr52@protonmail.com>

neutrinoceros previously approved these changes Sep 26, 2022

View reviewed changes

Missed a JSONdir --> datadir change

ccf4a04

mtryan83 dismissed neutrinoceros’s stale review via ccf4a04 September 27, 2022 17:45

neutrinoceros removed this from the 4.1.0 milestone Sep 28, 2022

Merge branch 'main' into updating_firefly

83259e3

# Conflicts: # setup.cfg

agurvich approved these changes Oct 3, 2022

View reviewed changes

neutrinoceros enabled auto-merge October 3, 2022 21:36

neutrinoceros approved these changes Oct 3, 2022

View reviewed changes

neutrinoceros merged commit f7eb388 into yt-project:main Oct 3, 2022

mtryan83 deleted the updating_firefly branch October 4, 2022 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating Firefly-dependent areas to match API changes and PyPI packaging #4143

Updating Firefly-dependent areas to match API changes and PyPI packaging #4143

mtryan83 commented Sep 24, 2022

welcome bot commented Sep 24, 2022

mtryan83 commented Sep 24, 2022

neutrinoceros left a comment

neutrinoceros left a comment

agurvich commented Sep 26, 2022

neutrinoceros commented Sep 27, 2022

mtryan83 commented Sep 27, 2022

neutrinoceros commented Sep 28, 2022

mtryan83 commented Oct 3, 2022 •

edited

Loading

agurvich left a comment

welcome bot commented Oct 3, 2022

Updating Firefly-dependent areas to match API changes and PyPI packaging #4143

Updating Firefly-dependent areas to match API changes and PyPI packaging #4143

Conversation

mtryan83 commented Sep 24, 2022

PR Summary

PR Checklist

welcome bot commented Sep 24, 2022

mtryan83 commented Sep 24, 2022

neutrinoceros left a comment

Choose a reason for hiding this comment

neutrinoceros left a comment

Choose a reason for hiding this comment

agurvich commented Sep 26, 2022

neutrinoceros commented Sep 27, 2022

mtryan83 commented Sep 27, 2022

neutrinoceros commented Sep 28, 2022

mtryan83 commented Oct 3, 2022 • edited Loading

agurvich left a comment

Choose a reason for hiding this comment

welcome bot commented Oct 3, 2022

mtryan83 commented Oct 3, 2022 •

edited

Loading