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

added AQUA.jl to Decapodes #216

Merged
merged 18 commits into from
Jun 6, 2024
Merged

added AQUA.jl to Decapodes #216

merged 18 commits into from
Jun 6, 2024

Conversation

quffaro
Copy link
Member

@quffaro quffaro commented Mar 16, 2024

Added AQUA.jl to test. This includes adding aqua.jl to test as well as removing stale exports. This does not fulfill requirements on #211

test/Manifest.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@GeorgeR227
Copy link
Collaborator

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

Removed Artifacts (since moved to CombSpaces) and commented out coordinates.jl (should be moved to CombSpaces)
I had to remove the Catlab only import of compile but I feel like it was worth it since it means we slim down a main dependency to a much smaller package.
@lukem12345
Copy link
Member

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

See #217

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

@GeorgeR227
Copy link
Collaborator

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.

This is now a helper file in examples but should ultimately be removed.
@lukem12345
Copy link
Member

See also test/coordinates.jl

@lukem12345
Copy link
Member

I would move it into the sw directory instead

@GeorgeR227
Copy link
Collaborator

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

@GeorgeR227 GeorgeR227 requested a review from jpfairbanks May 30, 2024 18:24
test/Manifest.toml Outdated Show resolved Hide resolved
ext/DecapodesCUDAExt.jl Show resolved Hide resolved
@GeorgeR227 GeorgeR227 requested a review from lukem12345 June 3, 2024 16:44
@lukem12345
Copy link
Member

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
Copy link
Member

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

@jpfairbanks
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@quffaro
Copy link
Member Author

quffaro commented Jun 3, 2024

@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
Copy link
Member

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

@GeorgeR227
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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.

Project.toml Show resolved Hide resolved
ext/DecapodesCUDAExt.jl Outdated Show resolved Hide resolved
src/operators.jl Outdated Show resolved Hide resolved
@jpfairbanks
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@GeorgeR227 GeorgeR227 merged commit d62d424 into main Jun 6, 2024
7 of 8 checks passed
@GeorgeR227 GeorgeR227 mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants