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

Purge .join() from mirgecom #566

Merged
merged 23 commits into from
Dec 15, 2021
Merged

Purge .join() from mirgecom #566

merged 23 commits into from
Dec 15, 2021

Conversation

thomasgibson
Copy link
Contributor

This PR does exactly what the title suggests. It removes all .join() calls!

Questions for the review:

  • Is the scope and purpose of the PR clear?
    • The PR should have a description.
    • The PR should have a guide if needed (e.g., an ordering).
  • Is every top-level method and class documented? Are things that should be documented actually so?
  • Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
  • Does the implementation do what the docstring claims?
  • Is everything that is implemented covered by tests?
  • Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

@MTCam MTCam mentioned this pull request Dec 11, 2021
8 tasks
@thomasgibson thomasgibson linked an issue Dec 13, 2021 that may be closed by this pull request
@thomasgibson thomasgibson marked this pull request as ready for review December 13, 2021 16:00
mirgecom/euler.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@majosm majosm 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, just added a couple of questions/comments.

Comment on lines 204 to 210
from arraycontext import map_array_container
from functools import partial
if not isinstance(field, DOFArray):
return map_array_container(
partial(filter_modally, dcoll, dd, cutoff, mode_resp_func), field
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the @obj_array_vectorize_n_args be removed after this change? Also, the docstring should probably be updated to reflect that fields and the return value can be object arrays now.

I think with inducer/arraycontext#128 it may be possible to implement this function as something along the lines of:

    def filter_scalar(f):
        dd_nodal = dof_desc.as_dofdesc(dd)
        dd_modal = dof_desc.DD_VOLUME_MODAL
        discr = dcoll.discr_from_dd(dd_nodal)
        actx = f.array_context
        modal_map = dcoll.connection_from_dds(dd_nodal, dd_modal)
        nodal_map = dcoll.connection_from_dds(dd_modal, dd_nodal)
        f = modal_map(f)
        f = apply_spectral_filter(actx, f, discr, cutoff, mode_resp_func)
        return nodal_map(f)

    from arraycontext import rec_map_array_container
    return rec_map_array_container(filter_scalar, field, leaf_class=DOFArray)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right that @obj_array_vectorize_n_args can be removed, thanks for pointing that out. I also updated the docs to reflect the changes: 74558fa

I think with inducer/arraycontext#128 it may be possible to implement this function as something along the lines of:

    def filter_scalar(f):
        dd_nodal = dof_desc.as_dofdesc(dd)
        dd_modal = dof_desc.DD_VOLUME_MODAL
        discr = dcoll.discr_from_dd(dd_nodal)
        actx = f.array_context
        modal_map = dcoll.connection_from_dds(dd_nodal, dd_modal)
        nodal_map = dcoll.connection_from_dds(dd_modal, dd_nodal)
        f = modal_map(f)
        f = apply_spectral_filter(actx, f, discr, cutoff, mode_resp_func)
        return nodal_map(f)

    from arraycontext import rec_map_array_container
    return rec_map_array_container(filter_scalar, field, leaf_class=DOFArray)

I think it's certainly an option. However, I'd prefer not to tackle that in this PR.

mirgecom/simutil.py Outdated Show resolved Hide resolved
mirgecom/simutil.py Show resolved Hide resolved
test/test_viscous.py Outdated Show resolved Hide resolved
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.

🎉 This is terrific! Thanks @thomasgibson!

@thomasgibson thomasgibson merged commit 0d4b5e1 into main Dec 15, 2021
@thomasgibson thomasgibson deleted the thg/purge-join branch December 15, 2021 14:31
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.

filer_modally should traverse array containers
3 participants