search

AlgebraicJulia / Decapodes.jl

A framework for composing and simulating multiphysics systems
https://algebraicjulia.github.io/Decapodes.jl/dev/
MIT License
46 stars 14 forks source link

added AQUA.jl to Decapodes #216

Closed quffaro closed 3 weeks ago

quffaro commented 3 months ago

Added AQUA.jl to test. This includes adding aqua.jl to test as well as removing stale exports. This does not fulfill requirements on https://github.com/AlgebraicJulia/Decapodes.jl/issues/211

GeorgeR227 commented 1 month ago

@lukem12345 Is the coordinates.jl file still relevant for Decapodes?

lukem12345 commented 1 month ago

@lukem12345 Is the coordinates.jl file still relevant for Decapodes?

See https://github.com/AlgebraicJulia/Decapodes.jl/issues/217

You can update the SW examples by following the pattern I set forth in the recent navier stokes simulations

GeorgeR227 commented 1 month ago

If those SW examples are the only uses of it, then I'll just move it to be a helper file in examples/sw for now. Those examples are very old anyway require a lot more refactoring than just this.

lukem12345 commented 1 month ago

See also test/coordinates.jl

lukem12345 commented 1 month ago

I would move it into the sw directory instead

GeorgeR227 commented 1 month ago

This closes #213. All Aqua tests are passing, even on CUDA.

Test Summary: | Pass  Total     Time
CUDA          |    9      9  4m06.8s
     Testing Decapodes tests passed 

shell> git log -1
commit c20cfb83c1866f3e465129314c53bc30738c6e75 (HEAD -> cm/aqua, origin/cm/aqua)
Author: GeorgeR227 <78235421+GeorgeR227@users.noreply.github.com>
Date:   Thu May 30 14:11:50 2024 -0400

    Resolve ambiguities in Decapodes
lukem12345 commented 4 weeks ago

In retrospect, it feels like it kind of defeats the point of using code quality tools, if we are just going to satisfy them by moving code out of the path that it sees. Namely, moving the coordinates code to the examples folder

jpfairbanks commented 4 weeks ago

Yes, we should just live with an imperfect score on the code quality, rather than remove code that isn't high enough quality.

jpfairbanks commented 4 weeks ago

Code coverage is a good example. We don't expect 100% coverage, we track the coverage ratio and make sure it isn't going down. Eventually it will go up.

GeorgeR227 commented 4 weeks ago

Right I agree but that wasn't my intention. The idea behind moving coordinates.jl is that the code shouldn't be in Decapodes, it doesn't help with compilation or anything. It just seems like a helper file.

GeorgeR227 commented 4 weeks ago

Besides, having this file around cluttered up the exports when essentially none of them were being used elsewhere.

quffaro commented 4 weeks ago

@jpfairbanks I agree that we should live with imperfect code coverage.

@lukem12345 I see your point about affecting code coverage. I've read #217 and I agree that we should factor out coordinates.jl.

@GeorgeR227 has already vocalized this, but I think the intent is to just get AQUA.jl to pass; otherwise if coordinates.jl were not marked for oblivion, AQUA.jl would make it incumbent on us to resolve whatever QA badness.

I have some bandwidth this week to complete #217.

lukem12345 commented 4 weeks ago

Yes there will still need to be some functionality around tangent bases that CRS does not currently offer AFAICT

GeorgeR227 commented 4 weeks ago

In any matter, this coordinate stuff shouldn't be in Decapodes proper. It doesn't seem to change the compilation process and is only used for initial conditions which is separate behavior.

GeorgeR227 commented 4 weeks ago

Also on the matter of code coverage, this file was already being test 100% I believe. Removing it is purely meant to keep Decapodes focused on the compilation aspect. This is why I also dumped over half the dependencies we used to have.

lukem12345 commented 4 weeks ago

It sounds like we need a plan for checking the tests, quality, and code coverage of functionality that does not directly affect the output of gensim, covered or uncovered as such functionality may be.

In this particular case, how about finding a suitable spot for this in CombinatorialSpaces?

GeorgeR227 commented 4 weeks ago

Yeah I agree but for now I think that we're at a good point, mainly since a lot of unused dependencies and exports have been removed.

My thoughts also turned to CombinatorialSpaces for this but I don't want this branch held up by figuring this out since these functions really were not being used anywhere.

jpfairbanks commented 4 weeks ago

All the docs are disabled here. That is just because you want to merge this before you merge #231, right?

@lukem12345 are the SW sims going to fail without the coordinate stuff?

jpfairbanks commented 4 weeks ago

Removing it is purely meant to keep Decapodes focused on the compilation aspect.

Decapodes needs to have some resources for people who are building simulations on triangulated manifolds. So coordinate transformations, data extractions, and visualization code belongs here, unless we want to make more packages like DecapodesPlots.jl.

GeorgeR227 commented 4 weeks ago

What I'm hoping to do right now is slim down Decapodes as much as I reasonably can and then if we find some other functionality is good to have in base Decapodes then we can add it.

GeorgeR227 commented 4 weeks ago

All the docs are disabled here. That is just because you want to merge this before you merge #231, right?

@lukem12345 are the SW sims going to fail without the coordinate stuff?

I've disabled the docs because I don't want to deal with disabling the workflow every time the actions trigger. I was planning on adding them back in before we merge since I don't want those changes here. The plan was actually to merge #231 and then this but having this first can work.