Some initial refactoring #122

archermarx · 2024-02-23T14:31:51Z

Addresses #121 in part, as well as making some other changes to the internals.
The main goal of this is to remove the vestiges of DifferentialEquations.jl. Previously, we had used that library for timestepping, and the entire design of the code was based on that. However, that led to some awkward design choices. Now that we no longer use DifferentialEquations, I would like to restructure the code in a way that makes more sense. This PR is a draft for now.

What's changed

The electron energy is no longer included in the state vector U. This saves about 3% of the computational time as we have reduced the number of rows we iterate through by 1
The electron pressure coupled scheme has been removed. While nice, it saw very little use and was restricted to singly-charged ions only. Including it required a lot of special-casing that made the code longer and more difficult to follow.
The test dependencies have been moved from test/Project.toml to the [extras] section of Project.toml. I have also pruned the test deps (removing Plots and others) so that the only heavy test dep is Symbolics, which we use for OVS.
Some unused arguments have been removed from the stage limiter
Remove the whole ODEProblem thing and associated structure
Remove references to DifferentialEquations from docs

Still to come (in later PRs)

Restructure params and cache. These were designed to work with the DifferentialEquations.jl interface, but it's difficult to see what's in there. Ideally, we'd replace with more descriptively-named structures (perhaps "simulation" and "data"?). This would be a pretty large breaking change, so perhaps we could include methods for accessing things the "old way" for a while
As part of the above, the Solution object could be made more useful with more descriptive fields. Having most of the important plasma properties hidden behind params.cache is not an ideal state of affairs
Fix restarts (maybe hold off until structure is more stable)

…urrently pass for some reaason, but may overhaul these

issue was electron pressure coupling, which I am removing

codecov-commenter · 2024-02-23T14:36:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.11%. Comparing base (9b9247a) to head (1e2619b).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
- Coverage   87.22%   84.11%   -3.11%     
==========================================
  Files          36       36              
  Lines        2058     2008      -50     
==========================================
- Hits         1795     1689     -106     
- Misses        263      319      +56

Files	Coverage Δ
src/HallThruster.jl	`100.00% <100.00%> (ø)`
src/numerics/flux.jl	`89.94% <100.00%> (-1.69%)`	⬇️
src/numerics/limiters.jl	`96.29% <100.00%> (+0.14%)`	⬆️
src/physics/fluid.jl	`100.00% <ø> (ø)`
src/physics/gas.jl	`100.00% <ø> (ø)`
src/physics/thermal_conductivity.jl	`63.63% <ø> (ø)`
src/simulation/boundaryconditions.jl	`100.00% <100.00%> (ø)`
src/simulation/configuration.jl	`78.68% <100.00%> (+0.42%)`	⬆️
src/simulation/electronenergy.jl	`96.59% <100.00%> (-0.04%)`	⬇️
src/simulation/geometry.jl	`95.58% <ø> (ø)`
... and 8 more

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

…e unused callback option

archermarx · 2024-02-26T15:38:57Z

I'm gonna mark this ready for review. @DecBrick, could you take a look?

DecBrick · 2024-02-26T22:06:26Z

docs/src/run.md

@@ -136,13 +136,13 @@ tasks.

 ### The `Solution` object

-Running a simulation returns a `HallThruster.Solution` object. This mimics a DifferentialEquations `DiffEqSolution` object and has the same fields:
+Running a simulation returns a `HallThruster.Solution` object, which has the the same fields:


Might be missing context, but the "same fields" seems to still be vestigial.

Yep, it is. Fixed!

DecBrick · 2024-02-26T22:14:31Z

src/numerics/flux.jl

            # Sound speeds
-            aL = sqrt((charge_factor * TeL + γ * kB * TL) / mi)
-            aR = sqrt((charge_factor * TeR + γ * kB * TR) / mi)
+            aL = sqrt((γ * kB * TL) / mi)


As the ions currently only have a single temperature, is it worth leaving in the left and right calculations for if an ion temperature model is eventually implemented?

Yeah, that's what I'm currently thinking

DecBrick · 2024-02-26T22:27:16Z

src/wall_loss_models/constant_sheath_potential.jl

@@ -14,7 +14,8 @@ function wall_power_loss(model::ConstantSheathPotential, U, params, i)
    (;z_cell, index, L_ch, config, cache) = params

    ne = cache.ne[i]
-    ϵ = U[index.nϵ, i] / ne
+    nϵ = cache.nϵ[i]
+    ϵ = nϵ / ne


Seems like nϵ is only called once in this file, could save an allocation unless it's a setup for future changes

naming a variable doesn't cause a memory allocation. we still need to compute \epsilon in order to get the wall power loss here

DecBrick · 2024-02-26T22:29:33Z

test/runtests.jl

@@ -80,3 +90,54 @@ end
        end
    end
 end
+


massive comment left at the bottom

removed, thanks

DecBrick

Looks good so far aside from the comments above.

For future steps, with regards to the params structure, it may be good to have a method to be able to call all of its keys in a concise manner from a solution object to allow the user to be able to see what properties are saved. A native way to do this in Julia may exist, but I haven't encountered it so far.

archermarx · 2024-02-26T22:35:03Z

Looks good so far aside from the comments below.

For future steps, with regards to the params structure, it may be good to have a method to be able to call all of its keys in a concise manner from a solution object to allow the user to be able to see what properties are saved. A native way to do this in Julia may exist, but I haven't encountered it so far.

i think fieldnames(typeof(sol.savevals[1])) would work, but i could add a saved_properties method that does that for you

DecBrick · 2024-02-26T22:44:30Z

Well it does, maybe not do a full method but make a note about that in the documentation as the documentation on what fields the object has may not always be up to date.

…rease

archermarx · 2024-02-26T23:05:11Z

I've added a HallThruster.saved_fields() function which will list all fields that get saved to sol.savevals. I've also updated the jupyter notebook to call this instead of using the static list of fields, and the docs so that this field is mentioned.

archermarx added 8 commits February 22, 2024 18:22

update precompile stuff

079e14d

fix weighted velocity average for electron upwinding

84c7a65

remove all refs to index.n_eps. note: ovs second order tests do not c…

798f73e

…urrently pass for some reaason, but may overhaul these

ovs tests passed

fa066f1

issue was electron pressure coupling, which I am removing

re-enable all OVS tests

0e74f3e

remove electron pressure-coupled sim

7305c25

shrink u by one column

58b84bc

comment out restart tests

1e2619b

archermarx added 2 commits February 23, 2024 09:45

remove ODEProblem and associated structure

327a6c5

some docs changes to remove refs to DifferentialEquations, also remov…

c61ae08

…e unused callback option

archermarx self-assigned this Feb 23, 2024

archermarx marked this pull request as ready for review February 26, 2024 15:40

archermarx requested a review from DecBrick February 26, 2024 15:40

DecBrick reviewed Feb 26, 2024

View reviewed changes

archermarx added 3 commits February 26, 2024 17:07

remove LinRangeWrapper

3260cd1

fix docs issue

e9be511

mark flux functions @inline

aebfe98

DecBrick reviewed Feb 26, 2024

View reviewed changes

DecBrick approved these changes Feb 26, 2024

View reviewed changes

fix runtime dispatch in savevals for reasonably large performance inc…

8072fa1

…rease

DecBrick approved these changes Feb 26, 2024

View reviewed changes

fix flux tests, add HallThruster.saved_fields(), and update docs

4d40efa

archermarx merged commit df19a3f into main Feb 26, 2024
4 checks passed

archermarx deleted the refactor-1 branch February 27, 2024 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some initial refactoring #122

Some initial refactoring #122

archermarx commented Feb 23, 2024 •

edited

Loading

codecov-commenter commented Feb 23, 2024 •

edited

Loading

archermarx commented Feb 26, 2024

DecBrick Feb 26, 2024

archermarx Feb 26, 2024

DecBrick Feb 26, 2024 •

edited

Loading

archermarx Feb 26, 2024

DecBrick Feb 26, 2024

archermarx Feb 26, 2024

DecBrick Feb 26, 2024

archermarx Feb 26, 2024

DecBrick left a comment •

edited

Loading

archermarx commented Feb 26, 2024

DecBrick commented Feb 26, 2024

archermarx commented Feb 26, 2024

@@ @@ -80,3 +90,54 @@ end @@
                       end
                   end
               end

Some initial refactoring #122

Some initial refactoring #122

Conversation

archermarx commented Feb 23, 2024 • edited Loading

What's changed

Still to come (in later PRs)

codecov-commenter commented Feb 23, 2024 • edited Loading

Codecov Report

archermarx commented Feb 26, 2024

DecBrick Feb 26, 2024

Choose a reason for hiding this comment

archermarx Feb 26, 2024

Choose a reason for hiding this comment

DecBrick Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

archermarx Feb 26, 2024

Choose a reason for hiding this comment

DecBrick Feb 26, 2024

Choose a reason for hiding this comment

archermarx Feb 26, 2024

Choose a reason for hiding this comment

DecBrick Feb 26, 2024

Choose a reason for hiding this comment

archermarx Feb 26, 2024

Choose a reason for hiding this comment

DecBrick left a comment • edited Loading

Choose a reason for hiding this comment

archermarx commented Feb 26, 2024

DecBrick commented Feb 26, 2024

archermarx commented Feb 26, 2024

archermarx commented Feb 23, 2024 •

edited

Loading

codecov-commenter commented Feb 23, 2024 •

edited

Loading

DecBrick Feb 26, 2024 •

edited

Loading

DecBrick left a comment •

edited

Loading