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

Rename kernels #9

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Rename kernels #9

wants to merge 4 commits into from

Conversation

geriJana
Copy link

Rename some of the kernels - their configure options - to match the subroutines names.

Renamed to match the subroutines name (weighted mrt).
Added a log message and abort for default mrt q27, as it is not
existing.
@geriJana
Copy link
Author

geriJana commented Feb 1, 2024

Q: weighted_mrt_advRel_d3q27_generic as d3q27 routine for mrt_generic. How to proceed with this? See code snippet below:

 case ('mrt_generic')
      select case (trim(layout))
      case ('d3q27')
        compute => weighted_mrt_advRel_d3q27_generic
      case ('d3q19')
        compute => mrt_advRel_d3q19_generic
      case default
        compute => mrt_advRel_generic
      end select

My suggestion: I create a new case (mrt_weighted_generic) for weighted_mrt_advRel_d3q27_generic and remove it from mrt_generic. Instead I'll add messages where necessary.

@geriJana
Copy link
Author

geriJana commented Feb 1, 2024

As I'm now working on it....

Q:
How should we name the correction routine bgk_HybridRecursiveRegularizedCorr, which only differs in the blending parameter sigma but is now called for different configure options as listed below:

    case ('hrr_bgk_corrected', 'prr_bgk_corrected', 'rr_bgk_corrected')
      select case (trim(layout))
      case ('d3q27')
        compute => bgk_HybridRecursiveRegularizedCorr_d3q27
      case ('d3q19')
        compute => bgk_HybridRecursiveRegularizedCorr_d3q19
      case ('d2q9')
        compute => bgk_HybridRecursiveRegularizedCorr_d2q9
      case default
        write(logUnit(1),*) 'Stencil '//trim(layout)//' is not supported yet!'
        call tem_abort()
      end select

Suggestion:
bgk_recursiveRegularizedCorrected_template
Please note also the change of word ordering and removed trailing capital later in routine name.

@geriJana
Copy link
Author

geriJana commented Feb 1, 2024

See comments above:

then we should change all the other recursive, regularized ones... See code below:

case ('prr_bgk')
      select case (trim(layout))
      case ('d3q27')
        compute => bgk_ProjectedRecursiveRegularized_d3q27

Removing trailing capital letter everywhere in routine names.

Q: Should it be

  1. bgk_projectedRecursiveRegularized
  2. projectedRecursiveRegularized_bgk
    ?

Renamed to match the subroutines name (weighted mrt incomp).
Added a log message and abort for default mrt q27, as it is not
existing.
@geriJana
Copy link
Author

geriJana commented Feb 1, 2024

@KannanMasilamani please have a look.

@geriJana
Copy link
Author

geriJana commented Feb 1, 2024

  • Update recheck cases to include also the renamed configure options for the schemes like mrt_weighted for comp and incomp.

Renamed to match the subroutines name (cumulant_d3q27_extended_fast).
@haraldkl
Copy link
Member

haraldkl commented Feb 1, 2024

My suggestion: I create a new case (mrt_weighted_generic) for weighted_mrt_advRel_d3q27_generic and remove it from mrt_generic. Instead I'll add messages where necessary.

Sounds good to me.

Renamed to match the guidelines. Because routine is not generic in terms of our
definition, where it means for 'fluid' and 'fluid_incomp'.
I also corrected some formatting things but stopped at some point. This
should be done seperatly.
@KannanMasilamani
Copy link
Contributor

Q: weighted_mrt_advRel_d3q27_generic as d3q27 routine for mrt_generic. How to proceed with this? See code snippet below:

 case ('mrt_generic')
      select case (trim(layout))
      case ('d3q27')
        compute => weighted_mrt_advRel_d3q27_generic
      case ('d3q19')
        compute => mrt_advRel_d3q19_generic
      case default
        compute => mrt_advRel_generic
      end select

My suggestion: I create a new case (mrt_weighted_generic) for weighted_mrt_advRel_d3q27_generic and remove it from mrt_generic. Instead I'll add messages where necessary.

Just to understand the generic, I thought we used generic keyword to compute kernels which are stencil independent. Now we have generic for d3q19 and d3q27, why?

@geriJana
Copy link
Author

geriJana commented Feb 1, 2024

Just to understand the generic, I thought we used generic keyword to compute kernels which are stencil independent. Now we have generic for d3q19 and d3q27, why?

'Now' is good. It's like that for a while. Besides, I don't know what you once decided.

@haraldkl
Copy link
Member

haraldkl commented Feb 1, 2024

Suggestion:
bgk_recursiveRegularizedCorrected_template
Please note also the change of word ordering and removed trailing capital later in routine name.

This is about the routine name not the configuration option, right? I wouldn't call it _template, just bgk_recursiveRegularizedCorrected sounds about right, or does this create a conflict somewhere? For the subroutine names capitalization doesn't really matter in Fortran (Though it's good to keep it consistent). From my point of view even bgk_rrc would suffice for the subroutine name.

@haraldkl
Copy link
Member

haraldkl commented Feb 1, 2024

Q: Should it be

bgk_projectedRecursiveRegularized
projectedRecursiveRegularized_bgk

Going by the other names, it would be bgk_projectedRecursiveRegularized, as far as I can see.

Comment on lines +598 to +599
write(logUnit(1),*) 'Checking admissibility of omegas for parametrized &
& Cumulant scheme'
Copy link
Member

Choose a reason for hiding this comment

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

Could we avoid using strings over two lines? Instead of doing that, I'd prefer to have:

Suggested change
write(logUnit(1),*) 'Checking admissibility of omegas for parametrized &
& Cumulant scheme'
write(logUnit(1),*) 'Checking admissibility of omegas for parametrized' &
& // ' Cumulant scheme'

@haraldkl
Copy link
Member

Some more discussion on this is found in #10.

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.

3 participants