-
Notifications
You must be signed in to change notification settings - Fork 27
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
RFC: possible Fields module #147
Conversation
Looks pretty good as an approach. Two comments
|
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).
The current version now allows one to specify the inputs as either:
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 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). |
The freespace inputs need a bit of thought. Other than that this is great, nice and easy to extend in the future. |
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. |
It's amazing what you can do when the camera is switched off! |
Co-Authored-By: Chris Brahms <38351086+chrisbrahms@users.noreply.github.com>
Co-Authored-By: Chris Brahms <38351086+chrisbrahms@users.noreply.github.com>
Co-Authored-By: Chris Brahms <38351086+chrisbrahms@users.noreply.github.com>
Co-Authored-By: Chris Brahms <38351086+chrisbrahms@users.noreply.github.com>
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. |
Incidentally I checked coverage and with |
Just looking at this now.
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. |
But the excellent vscode extension https://marketplace.visualstudio.com/items?itemName=ryanluker.vscode-coverage-gutters |
oooh shiny. how does that work, you run Coverage.jl to generate the lcov and the extension finds it? |
This was an issue with the time resolution of the grid since Alternatively you could just check that the CEO is equal to the desired value to within less than the grid spacing. |
I like your solution, thanks! |
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) |
All tests were run fine previous to your commit which only edited tests. |
Only tested with
test_main
andtest_main_env
so far.