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

Feature/fortran api redistribution #160

Merged
merged 16 commits into from
Nov 9, 2023

Conversation

sbrdar
Copy link
Collaborator

@sbrdar sbrdar commented Oct 31, 2023

Add Fortran API for atlas::Redistribution
(with Willem D.)

@FussyDuck
Copy link

FussyDuck commented Oct 31, 2023

CLA assistant check
All committers have signed the CLA.

@sbrdar sbrdar requested a review from wdeconinck October 31, 2023 15:35
Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

Very good! I have some minor suggestions only.
Further to in-code suggestions, please in atlas_module, add a line to export atlas_redistribution type.

type(atlas_Redistribution) :: this
type(atlas_Config) :: config
config = empty_config()
call config%set("type", "RedistributeGeneric")
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this line call config%set("type", "RedistributeGeneric")
and allow to pass down an empty config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This config%set has been removed now. I had to create a default case to be "RedistributeGeneric".

const functionspace::FunctionSpaceImpl* fspace1, const functionspace::FunctionSpaceImpl* fspace2,
const eckit::Configuration* config) {
ATLAS_ASSERT(config != nullptr);
std::string type;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string type;
std::string type = detail::RedistributeGeneric::static_type();

and add include #include "atlas/redistribution/detail/RedistributeGeneric.h" below line 16

Copy link
Member

Choose a reason for hiding this comment

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

This catches passing down the empty config in Fortran

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (5ea733a) 79.40% compared to head (adbb751) 79.63%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #160      +/-   ##
===========================================
+ Coverage    79.40%   79.63%   +0.22%     
===========================================
  Files          822      847      +25     
  Lines        62223    62715     +492     
===========================================
+ Hits         49410    49943     +533     
+ Misses       12813    12772      -41     
Files Coverage Δ
src/atlas/mesh/MeshBuilder.cc 91.08% <100.00%> (+1.37%) ⬆️
src/atlas/mesh/MeshBuilder.h 100.00% <100.00%> (ø)
...s/redistribution/detail/RedistributionInterface.cc 100.00% <100.00%> (ø)
src/atlas_f/atlas_module.F90 36.11% <ø> (ø)
...tests/mesh/fctest_mesh_triangular_mesh_builder.F90 100.00% <100.00%> (ø)
...rc/tests/mesh/test_mesh_triangular_mesh_builder.cc 100.00% <100.00%> (ø)
src/tests/redistribution/fctest_redistribution.F90 100.00% <100.00%> (ø)
...s_f/redistribution/atlas_Redistribution_module.F90 96.42% <96.42%> (ø)
src/atlas/mesh/detail/MeshBuilderIntf.cc 69.23% <69.23%> (ø)
src/atlas_f/mesh/atlas_MeshBuilder_module.F90 66.66% <66.66%> (ø)

... and 169 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wdeconinck
Copy link
Member

Please also address failed compilation with Intel compiler, see e.g. https://github.com/ecmwf/atlas/actions/runs/6708816197/job/18230465722?pr=160#step:14:2303

@@ -7,6 +7,15 @@

if( atlas_HAVE_ATLAS_FUNCTIONSPACE )

Copy link
Member

Choose a reason for hiding this comment

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

The add_fctest command needs to be guarded by

if( HAVE_FORTRAN )
  add_fctest(...)
endif()

Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

All good now. Thanks!

@wdeconinck wdeconinck force-pushed the feature/FortranAPI_redistribution branch from 27e849d to adbb751 Compare November 9, 2023 15:03
@wdeconinck wdeconinck merged commit 72e1846 into develop Nov 9, 2023
90 of 91 checks passed
@wdeconinck wdeconinck deleted the feature/FortranAPI_redistribution branch November 9, 2023 15:35
wdeconinck added a commit that referenced this pull request Dec 11, 2023
* release/0.36.0: (25 commits)
  Update Changelog
  Version 0.36.0
  Add atlas_Redistribution to atlas_module
  Performance improvement in EqualAreaPartitioner for StructuredGrid
  Add EqualAreaPartitioner which is geometry based compared to EqualRegionsPartitioner which is loadbalanced
  Add fallback mechanism to MatchingMeshPartitionerLonLatPolygon
  Fix MatchingMeshPartitionerBruteForce
  Fix MDPI_gulfstream: 180 degrees phase shift corrected
  Feature/fortran api init functions (#165)
  Fix failing tests for IntelLLVM Release builds
  GHA: Install intel-oneapi with mpi-devel for access to MPI headers and Fortran modules
  Fix intel compiler version in github actions, and add IntelLLVM
  Add compile flag -fno-finite-math-only for Intel LLVM compiler (icx, icpx)
  Add Fortran API for functionspace::CellColumns (#164)
  Cleanup
  Fix logic
  Fix interpolation warnings
  Add Fortran interface for Redistribution (#160)
  Add atlas_TriangularMeshBuilder with Fortran interface for serial meshes
  Reduce header dependency on atlas_io to prepare for eckit_codec adaptor library
  ...
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.

4 participants