gridapapps / GridapGeosciences.jl

Gridap drivers for geoscience applications

MIT License

35 stars 3 forks source link

Removed PolynomialMapCubedSphereDiscreteModel and PolynomialMapCubedSphereTriangulation types. #35

Closed amartinhuertas closed 2 years ago

amartinhuertas commented 2 years ago

They are not actually needed. We can provide the same functionality using Gridap types.

davelee2804 commented 2 years ago

Hi @amartinhuertas , is this PR going to effect the use of GridapHybrid on the cubed sphere?

amartinhuertas commented 2 years ago

Hi @amartinhuertas , is this PR going to effect the use of GridapHybrid on the cubed sphere?

Hi @davelee2804 ! I was revisiting the data structures for CubedSphere meshing (towards refreshing what we did) and I realized that there are two types which are just wrappers of Gridap types. Thus, we can eliminate them. This does not solve per-se the current problem we have with GridapHybrid on the cubed sphere.

davelee2804 commented 2 years ago

Thanks for the confirmation @amartinhuertas

amartinhuertas commented 2 years ago

For the records, MPI tests are failing. https://github.com/gridapapps/GridapGeosciences.jl/runs/7143096336?check_suite_focus=true#step:9:102

However, I realized that exactly the same error is in the hdg_adv branch. https://github.com/gridapapps/GridapGeosciences.jl/runs/7129780507?check_suite_focus=true#step:9:102

Thus, the changes introduced in this PR seem to be inocent for this failing tests.

amartinhuertas commented 2 years ago

For the records, the following commit in main might circumvent (temporarily) the failing MPI Tests https://github.com/gridapapps/GridapGeosciences.jl/commit/7cf147eac1adab2ded0022f08428ac486d8efa74

amartinhuertas commented 2 years ago

Ok, sequential tests pass. This seems to confirm that the two data structures I have eliminated in this PR are not actually needed.

davelee2804 commented 2 years ago

Thanks @amartinhuertas , I'll pull that in...

amartinhuertas commented 2 years ago

Thanks @amartinhuertas , I'll pull that in...

Let us see if the tests pass first in this branch. We have to pull that in the ADV-HDG branch with care. There might be conflicts. I can do it, no worries.

davelee2804 commented 2 years ago

OK, thanks Alberto!

codecov-commenter commented 2 years ago

Codecov Report

Merging #35 (d790772) into master (7cf147e) will not change coverage. The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master     #35   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          18      18           
  Lines        1258    1246   -12     
======================================
+ Misses       1258    1246   -12

Impacted Files	Coverage Δ
src/CubedSphereDiscreteModels.jl	`0.00% <0.00%> (ø)`
src/CubedSphereTriangulations.jl	`0.00% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7cf147e...d790772. Read the comment docs.