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

Make nel_1d mean what it says #287

Merged
merged 13 commits into from May 3, 2021
Merged

Conversation

majosm
Copy link
Collaborator

@majosm majosm commented Mar 15, 2021

The n parameter in generate_regular_rect_mesh is the number of points in 1D, so nel_1d is currently kind of misnamed. This PR fixes it.

@MTCam
Copy link
Member

MTCam commented Mar 15, 2021

A thousand thumbs up on this, but: shouldn't it be renamed to npts_1d instead of left as nel_1d just to be corrected at the call sites where it's used?

Edit: My thinking is that instead of actually changing all the grids, that the grids can be left the same. The grid interface actually expects the argument to be num points, so I find that leaving the variable named nel_1d is still misleading.

@inducer
Copy link
Contributor

inducer commented Mar 15, 2021

Whoops! I feel (at least partly) responsible here. Thanks @majosm for cleaning this up! (Could you do the same in the grudge and meshmode examples, if that's not asking too much?) FWIW I agree with @MTCam that renaming the variable to npts_1d is probably better.

@majosm
Copy link
Collaborator Author

majosm commented Mar 15, 2021

I still like thinking of the problem size in terms of numbers of elements instead of points. It makes it easier to multiply the mesh resolution. Also makes code like this simpler (from #272):

 nel_1d_0 = 5
 for hn1 in [1, 2, 3, 4]:

-    nel_1d = hn1 * (nel_1d_0 - 1) + 1
-    h = 1/(nel_1d-1)
+    nel_1d = hn1 * nel_1d_0
+    h = 1/nel_1d

If the +1s are awkward, could we possibly add an nel= parameter to generate_regular_rect_mesh (and then maybe deprecate n=?)?

@inducer
Copy link
Contributor

inducer commented Mar 15, 2021

I still like thinking of the problem size in terms of numbers of elements instead of points.

Actually, that's a fair point.

If the +1s are awkward, could we possibly add an nel= parameter to generate_regular_rect_mesh (and then maybe deprecate n=?)?

I like that idea! We could (not saying we should) offer npts= and nel= to make the interface clearer. Or just nel. It's not like it's hard to convert.

@MTCam
Copy link
Member

MTCam commented Mar 15, 2021

I still like thinking of the problem size in terms of numbers of elements instead of points.

Actually, that's a fair point.

If the +1s are awkward, could we possibly add an nel= parameter to generate_regular_rect_mesh (and then maybe deprecate n=?)?

I like that idea! We could (not saying we should) offer npts= and nel= to make the interface clearer. Or just nel. It's not like it's hard to convert.

+1's to everything here (even if it's awkward). I also prefer to think about number of elements, and would love it to have this distinction acknowledged by the interface.

Copy link
Member

@MTCam MTCam left a comment

Choose a reason for hiding this comment

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

Still not a giant fan of that it changes the previous grids, but still prefer it over the previous thing.

@majosm majosm marked this pull request as draft March 15, 2021 20:29
@majosm majosm marked this pull request as ready for review May 3, 2021 18:30
@majosm majosm requested a review from inducer May 3, 2021 18:31
Copy link
Contributor

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks!

@majosm majosm merged commit 76b7975 into illinois-ceesd:main May 3, 2021
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.

None yet

3 participants