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

ENH: Remove duplicate code in boxlib #3615

Merged
merged 2 commits into from
Nov 1, 2021

Conversation

cphyc
Copy link
Member

@cphyc cphyc commented Oct 20, 2021

Removes duplication in boxlib frontend.

This is one step aimed at minimizing code duplication.

The duplicates are spotted using

pylint --disable=all --enable=duplicate-code yt/

Note that I have not attempted to fix all duplicate here.

I am not fully satisfied with the way I have de-duplicated code with chombo/boxlib. Any input is welcome!
I'd also be happy to split the PR in multiple chunks.

@neutrinoceros
Copy link
Member

Hey, there's a lot going on here, but it sounds like a good idea.
I'd be interested in reviewing specifically the geometry handler dedups, and I'll admit I feel much less confident about deduplicating frontends (and boxlib in particular is a minefield). Maybe you'd like to separate these contributions into different PRs ?

@cphyc cphyc force-pushed the remove-duplicate-code branch from 1c6509d to f92abac Compare October 20, 2021 18:23
@cphyc cphyc force-pushed the remove-duplicate-code branch from f92abac to f719788 Compare October 28, 2021 10:25
@cphyc cphyc force-pushed the remove-duplicate-code branch from f719788 to 3900ecb Compare October 28, 2021 10:27
@cphyc cphyc changed the title ENH: Remove duplicate code ENH: Remove duplicate code in boxlib Oct 28, 2021
@cphyc cphyc added enhancement Making something better refactor improve readability, maintainability, modularity labels Oct 28, 2021
@cphyc cphyc marked this pull request as ready for review October 28, 2021 10:29
@neutrinoceros
Copy link
Member

Looks like you may have prematurely opened this for review, CI is failing.

@matthewturk
Copy link
Member

@zingale Can you peek at this real quick? It looks OK to me.

@zingale
Copy link
Member

zingale commented Nov 1, 2021

it looks good to me. I haven't explicitly tested it (and don't really work with particles).

@matthewturk matthewturk merged commit 7704cd0 into yt-project:main Nov 1, 2021
@cphyc cphyc deleted the remove-duplicate-code branch November 2, 2021 10:51
@neutrinoceros
Copy link
Member

neutrinoceros commented Aug 13, 2022

Seems like we haven't been testing Orion2 data with sink files, and this patch broke that (#4078)
I get lost very quickly trying to isolate the part that needs fixing so I'm going to propose a simple revert.

edit: ah, of couse an automatic revert doesn't work because the patch is too big and too old. Welp, looks like we're in for an actual fix then.

neutrinoceros added a commit to neutrinoceros/yt that referenced this pull request Aug 15, 2022
…e-code"

This reverts commit 7704cd0, reversing
changes made to b7dfe4a.
@neutrinoceros neutrinoceros mentioned this pull request Aug 15, 2022
neutrinoceros added a commit to neutrinoceros/yt that referenced this pull request Aug 15, 2022
…e-code"

This reverts commit 7704cd0, reversing
changes made to b7dfe4a.
matthewturk added a commit that referenced this pull request Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better refactor improve readability, maintainability, modularity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants