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

Support multiple volumes in a discretization collection #236

Closed
wants to merge 15 commits into from

Conversation

inducer
Copy link
Owner

@inducer inducer commented Mar 17, 2022

This isn't finished yet. Missing:

cc @majosm

Closes #104.

@inducer inducer force-pushed the multi-volume branch 2 times, most recently from 752aa81 to b678028 Compare March 17, 2022 06:19
@inducer
Copy link
Owner Author

inducer commented Mar 17, 2022

@majosm I think this is headed towards being not totally broken. Could you take a look to see how things look to you and look into the remaining issues above?

@inducer
Copy link
Owner Author

inducer commented Mar 17, 2022

The changes discussed in inducer/meshmode#308 (comment) are now in. Ready to take a look, @majosm. (assuming CI is happy 🤞)

@majosm
Copy link
Collaborator

majosm commented Apr 1, 2022

@inducer Any particular reason to be using

def func(..., volume_dd=None):
    if volume_dd is None:
        volume_dd = DD_VOLUME_ALL
    ...

vs.

def func(..., volume_dd=DD_VOLUME_ALL)?

?

I don't remember the exact guidelines for this. (I guess mutable objects should use the first form... not sure what else.) Does it make a difference here?

@inducer
Copy link
Owner Author

inducer commented Apr 1, 2022

I agree that mutable objects should use the first form. The other aspect that I run into is that if there's multiple layers of functions calling each other, and only the "innermost" function looks at the argument, then I find it cleaner to use None instead of redundantly specifying the default. If you're not in either of those cases, I'm not sure I have a preference.

@majosm majosm mentioned this pull request Sep 23, 2022
2 tasks
@majosm
Copy link
Collaborator

majosm commented Sep 26, 2022

Superseded by #280 and #285.

@majosm majosm closed this Sep 26, 2022
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.

DiscretizationCollection constructor should do no work
2 participants