RFC: possible Fields module

[jtravs]

Only tested with test_main and test_main_env so far.

[chrisbrahms]

Looks pretty good as an approach. Two comments

1. For the energy functions, one thing I didn't change in More statistics functions #115 for backward compatibility is the t/ω argument--it's redundant since a) we create the function with the Grid available and b) for at least some of them, the prefactors only make sense for that grid. Would we ever want to be able to use these functions without creating a Grid first? If so we should also add methods to do that.

2. For multimode inputs, one way to reduce the endless nested tuples would be to add a + method for Fields so that instead of (Field1, Field2...) we can write Field1 + Field2

[jtravs]

1. For the energy functions, one thing I didn't change in More statistics functions #115 for backward compatibility is the t/ω argument--it's redundant since a) we create the function with the Grid available and b) for at least some of them, the prefactors only make sense for that grid. Would we ever want to be able to use these functions without creating a Grid first? If so we should also add methods to do that.

OK I removed that (we can always add a new version when we have an actual use case, that way we are designing for a real need).

For multimode inputs, one way to reduce the endless nested tuples would be to add a + method for Fields so that instead of (Field1, Field2...) we can write Field1 + Field2

The current version now allows one to specify the inputs as either:

1. A single AbstractField instance (assumed to be in the first mode)
2. A tuple of AbstractField instances (assumed to all be in the 1st mode)
3. A tuple of NamedTuples, each with a mode and fields field specifying which fields go where

I think this simplifies the most common cases (especially a single field in the first mode), while allowing full control. We could add types to handle this, but maybe we see how well this works first?

I have also made all of the energy_modal, energy_radial etc. a single energyfuncs method using dispatch to select what to return.

At the moment I haven't fixed up the tests/examples as I want your input before proceeding (what will be quite a lot of changes).

[chrisbrahms]

The freespace inputs need a bit of thought. Other than that this is great, nice and easy to extend in the future.

[jtravs]

So this is now complete in terms of functionality (better Field interface, works for modeavg, modal, radial and 3D). I need to fix the tests and examples though (the freespace ones are fixed). This will take a bit of work and I don't have time now, so this'll be on hold a bit.

[jtravs]

between teaching meetings

It's amazing what you can do when the camera is switched off!

[jtravs]

OK, everything should be fixed and tested now. Only the elliptical polarisation examples need more work, but they were in a bad state anyway.

However, my tests fail for nonzero CEO phase, starting at line 347 in test_fields.jl @chrisbrahms can you have a look when you get a chance. My method of calculating the CEO is a bit poor, which may explain this. Or something worse is happening. 0 CEO works fine.

[jtravs]

Incidentally I checked coverage and with ENV["DISABLE_AMEND_COVERAGE_FROM_SRC"]="yes" we get 92%, but with the default we get something closer to 50%. I don't believe the latter, I think our tests are pretty good. It's a bit annoying that it is not possible to get good coverage stats.

[chrisbrahms]

However, my tests fail for nonzero CEO phase, starting at line 347 in test_fields.jl @chrisbrahms can you have a look when you get a chance. My method of calculating the CEO is a bit poor, which may explain this. Or something worse is happening. 0 CEO works fine.

Just looking at this now.

Incidentally I checked coverage and with ENV["DISABLE_AMEND_COVERAGE_FROM_SRC"]="yes" we get 92%, but with the default we get something closer to 50%. I don't believe the latter, I think our tests are pretty good. It's a bit annoying that it is not possible to get good coverage stats.

I'd guess that code with a lot of closures is particularly prone to the coverage issues at the moment? Lots of small functions that get compiled away. Not sure I believe 92% either though.

[jtravs]

But the excellent vscode extension https://marketplace.visualstudio.com/items?itemName=ryanluker.vscode-coverage-gutters
shows all code not covered by tests, and I have already found several missing parts, so at least that is useful.

[chrisbrahms]

oooh shiny. how does that work, you run Coverage.jl to generate the lcov and the extension finds it?

[jtravs]

Yes. It is actually excellent. I've found quite a few untested bits of code already. Usually missed branches in functions that we do test, e.g. (green is tested, red is untested)

[chrisbrahms]

However, my tests fail for nonzero CEO phase, starting at line 347 in test_fields.jl @chrisbrahms can you have a look when you get a chance. My method of calculating the CEO is a bit poor, which may explain this. Or something worse is happening. 0 CEO works fine.

This was an issue with the time resolution of the grid since getceo uses argmax. If you make the CEO a multiple of the grid spacing it works.

Alternatively you could just check that the CEO is equal to the desired value to within less than the grid spacing.

[jtravs]

I like your solution, thanks!
I think this is now pretty much complete.

[chrisbrahms]

Yes I'm very happy with this. Monumental editing effort there 👍

Assmuming tests pass go ahead and merge. (very annoyed that the test suite won't run on PRs)

[jtravs]

All tests were run fine previous to your commit which only edited tests.