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.

Sign up for GitHub

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

Jump to bottom

[BOXLIB] consistent parsing for periodicity parameters #2792

Merged
merged 9 commits into from
Oct 9, 2020
Merged

[BOXLIB] consistent parsing for periodicity parameters #2792

merged 9 commits into from
Oct 9, 2020

Conversation

neutrinoceros
Copy link
Member

PR Summary

This supersedes #2787 and adresses the issue at the frontend level. Turns out there is actually more than one place where periodicity is incorrectly or inconsistently parsed.

PR Checklist

  • pass flake8 yt/
  • pass isort . --check --diff
  • pass black --check yt/

@neutrinoceros neutrinoceros added bug code frontends Things related to specific frontends labels Jul 28, 2020
@neutrinoceros neutrinoceros mentioned this pull request Jul 28, 2020
Closed
5 tasks
neutrinoceros
neutrinoceros commented Jul 28, 2020
View reviewed changes
Copy link
Member Author

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I "reviewed" myself as a means to document the changes, but obvious this shouldn't count as a review !

yt/frontends/boxlib/data_structures.py Outdated
@@ -733,7 +733,7 @@ def _parse_cparams(self):
elif param == "amr.ref_ratio":
vals = self.refine_by = int(vals[0])
elif param == "Prob.lo_bc":
vals = self.periodicity = ensure_tuple([i == 0 for i in vals.split()])
vals = self.periodicity = ensure_tuple([p == "1" for p in vals.split()])
Copy link
Member Author

Choose a reason for hiding this comment

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

this line is clearly bugged since it relies on comparing a string to an integer. From what I think I understood, this should be the correct version for it.

yt/frontends/boxlib/data_structures.py Outdated
Comment on lines 1263 to 1254
self.periodicity = [True, True, True]
for i, axis in enumerate("xyz"):
try:
self.periodicity[i] = self.parameters["bc%s_lo" % axis]
except KeyError:
break
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is just a refactor to avoid repeated code (bis).

yt/frontends/boxlib/data_structures.py Outdated
Comment on lines 1647 to 1653
self.periodicity = [True] * 3
try:
is_periodic = self.parameters["geometry.is_periodic"].split()
self.periodicity[: len(is_periodic)] = [p == "1" for p in is_periodic]
except KeyError:
pass
self.periodicity = ensure_tuple(self.periodicity)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the part addressed originally in #2787

@neutrinoceros neutrinoceros changed the title [BOXLIB] consistent parsing for periodicity parameter [BOXLIB] consistent parsing for periodicity parameters Jul 28, 2020
ax3l
ax3l reviewed Jul 28, 2020
View reviewed changes
yt/frontends/boxlib/data_structures.py
@@ -1650,11 +1644,13 @@ def _parse_parameter_file(self):
self.parameters[l[0].strip()] = l[1].strip()

# set the periodicity based on the integer BC runtime parameters
is_periodic = self.parameters["geometry.is_periodic"].split()
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is the crash that #2787 fixes.

neutrinoceros
neutrinoceros commented Jul 30, 2020
View reviewed changes
yt/frontends/boxlib/data_structures.py Outdated
@@ -621,7 +621,7 @@ class BoxlibDataset(Dataset):
_output_prefix = None

# THIS SHOULD BE FIXED:
Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewturk can I have your review on this comment you wrote ? I don't know what was there to fix (default value, type, anything else ?) and if that comment should be cleaned up or updated.

@munkm
Copy link
Member

munkm commented Aug 10, 2020

I think because this PR supersedes another one then it is important that @ax3l and @zingale review and approve it. What are both of your thoughts on this solution?

ax3l
ax3l reviewed Sep 15, 2020
View reviewed changes
Copy link
Contributor

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one question.

@zingale and @atmyers do you like to give this a 2nd view? :)

yt/frontends/boxlib/data_structures.py Outdated
tmp[2] = False
self.periodicity = ensure_tuple(tmp)
self.periodicity = list(self.periodicity)
self.periodicity[1:] = False
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I don't understand this line easily, can you please clarify or add a comment? :)

Copy link
Member Author

@neutrinoceros neutrinoceros Sep 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for reviewing this ! In light of recent developments (especially #2893, where I essentially remove ensure_tuple), I just gave this another go. The specific line you're commenting here is now gone, and now periodicity should be only parsed once in BoxlibDataset._parse_cparams, in a dimensionality-agnostic way. Is it clearer now ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ax3l We'll wait for your reply before this is merged :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it clearer now ?

yes, thanks a lot.

@neutrinoceros neutrinoceros force-pushed the boxlib_periodicity branch 2 times, most recently from 0c41f93 to c14baa5 Compare September 15, 2020 09:22
@zingale
Copy link
Member

zingale commented Sep 16, 2020

I can confirm that this works with Castro and correctly detects the periodicity (at least for the data set I tried).

@neutrinoceros
Copy link
Member Author

Thank you @zingale ! 🎉

@neutrinoceros
Copy link
Member Author

hi @ax3l, I'm sorry to ask again but I couldn't find another way to contact you: do you approve of this PR in the current state ? thanks !

ax3l
ax3l approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@munkm munkm merged commit fefdf4e into yt-project:master Oct 9, 2020
@neutrinoceros neutrinoceros deleted the boxlib_periodicity branch October 9, 2020 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ax3l ax3l ax3l approved these changes

@zingale zingale zingale approved these changes

@matthewturk matthewturk Awaiting requested review from matthewturk

Assignees
No one assigned
Labels
bug code frontends Things related to specific frontends
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@neutrinoceros @munkm @zingale @ax3l