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.

Sign up for GitHub

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

Jump to bottom

Separate NA phases #172

Merged
merged 20 commits into from
Jul 3, 2024
Merged

Separate NA phases #172

merged 20 commits into from
Jul 3, 2024

Conversation

auggiemarignier
Copy link
Contributor

We want to allow the user to just perform the direct search phase of the NA.

Initial idea (quick fix) is to simply make all the parameters related to the appraisal phase optional, and if these are not set simply return the results of the direct search phase.

If we want, we can ultimately have two separate inference tools (more complete fix). A use case for this would be a user who has some samples from a posterior distribution obtained with some other method, and wants to increase their sample size. It would also be slightly cleaner for a user who just wants the NA direct search phase. One issue I see with this is that it would effectively double the work of a user who just wants to do the two phases of the NA, as they would need to essentially go through the CoFI workflow twice.

@auggiemarignier
Copy link
Contributor Author

Need to give an option to run the direct search in serial, e.g. if the forward model is not pickleable (pygimli....)

@auggiemarignier
feat: add option to run direct search serially
565594b
@auggiemarignier
test: parameterise inversion_options
7b44908
@auggiemarignier
feat: create three different Neighpy versions
6af87d4 
Squashed commit of the following:  commit
1948aefa56813a9608789ac3a6e9846eb4fe8fe5 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 20:05:10 2024 +1000
remove option to run only direct search in Neighpy  commit
554cea0efa2f125a1e80028e1ed4f15613c80306 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 17:27:20 2024 +1000
call static methods in combined class  commit
8afd413c8dbf5ab5d968f26cb7f6949affd83b67 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 16:59:52 2024 +1000
get both phases running (parallel appraisal hangs)  commit
f4749d0f9d4f45eb84d9dc3dc2c3861771b80315 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 16:15:46 2024 +1000
verify shapes of provided samples and logppd  commit
29abbeb35d0a58783cefbce674a360bb0f3fe11b Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 16:01:21 2024 +1000
use more specific option names  commit
02d79d69492585e590ea8fa433bb1e997f551d61 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 15:56:05 2024 +1000
add validation tests for the three solvers  commit
9e2c56f32c1f765a29e251176d1ebababc4d8400 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 15:41:15 2024 +1000
create separate tools for the two phases
@auggiemarignier
test: refactor tests
bc6acdd 
Squashed commit of the following:  commit
b5223d267f5fdb4e25554f27a33899de567726af Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 21:55:25 2024 +1000
add some more or less helpful typehints  commit
efc5307a04b8292038c4cd1890aa17fe8fbaa39a Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 21:51:30 2024 +1000
put valid options into a fixture  commit
1291a1f0c49b3c19952c830180f0b8d2734cc1d9 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 21:39:11 2024 +1000
test run Inversion object  commit
54370f8f8878255742470bb47351f99b2669ab71 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 21:38:57 2024 +1000
add solvers to tools table  commit
a34471a7f8b46d87716e0b7d4455b8441fb90066 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 21:27:33 2024 +1000
parameterise test_call  commit df4816681d0c8d8c2401e89f327217138af520c5
Author: auggiemarignier <[email protected]> Date:   Mon Jun
17 21:22:28 2024 +1000      parameterise invalid input shapes  commit
59df96d4b36c71223ee8452cde1387f9bae774aa Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 21:09:34 2024 +1000
test empty inversionoptions  commit
0d05b26d2bfce343facd80197968e3684931b6ae Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 21:03:44 2024 +1000
parameterise valid options checks  commit
d93c9cc275e878158946a1c9832b223499bf4d51 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 20:52:26 2024 +1000
parameterise empty baseproblem tests  commit
f66f910d4f03e3e827a082f7ea38a7db94b35019 Author: auggiemarignier
<[email protected]> Date:   Mon Jun 17 20:42:55 2024 +1000
add pytest-lazy-fixture  commit 4c54f63adc696533b31563294a32e792e19a89d0
Author: auggiemarignier <[email protected]> Date:   Mon Jun
17 20:31:17 2024 +1000      work with solver object instead of Inversion
object  commit 23f32aa508138829a52d606d9a02ee0c8312bf2e Author:
auggiemarignier <[email protected]> Date:   Mon Jun 17
20:25:42 2024 +1000      add fixtures for the three classes
@auggiemarignier
Merge branch 'main' into feature/neighpydirectsearch
ad9afb3
@auggiemarignier
Copy link
Contributor Author

auggiemarignier commented Jun 17, 2024 •
edited
Loading

I've now split this into 3 different tools:

  1. Neighpy - runs both phases
  2. NeighpyI - runs only direct search phase
  3. NeighpyII - runs only appraisal phase

Neighpy calls the methods of NeighpyI and NeighpyII so there's a fair bit of coupling going on but I think this is the most efficient way to do it to avoid unnecessary code duplication.

NeighpyII takes an initial_ensemble and corresponding logppd in InversionOptions. These can be from any other sampler.

Still TODO:

  • NeighpyII hangs when running in parallel for some reason, but Neighpy is fine...
  • Examples of all three in action

@auggiemarignier auggiemarignier marked this pull request as draft June 17, 2024 12:17
@auggiemarignier auggiemarignier changed the title Feature/neighpydirectsearch Separate NA phases Jun 17, 2024
@auggiemarignier auggiemarignier marked this pull request as ready for review July 1, 2024 07:15
@auggiemarignier auggiemarignier removed the request for review from msambridge July 1, 2024 07:15
@auggiemarignier
Copy link
Contributor Author

@jwhhh this is ready if you want to check it

jwhhh
jwhhh approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@jwhhh jwhhh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code all looks good to me! We can merge it once the version restriction is updated.

@auggiemarignier
Copy link
Contributor Author

which version restriction?

@jwhhh
Copy link
Member

jwhhh commented Jul 3, 2024

neighpy=0.1.2 seems to fail the tests as there's no parallel argument in the search stage. Which version onwards of neighpy would you recommend cofi to write in pyproject.toml?

@jwhhh
Copy link
Member

jwhhh commented Jul 3, 2024

It all looks good to me now. Please merge once you think it's ready

@auggiemarignier auggiemarignier merged commit dd20487 into main Jul 3, 2024
5 checks passed
@auggiemarignier auggiemarignier deleted the feature/neighpydirectsearch branch July 3, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwhhh jwhhh jwhhh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@auggiemarignier @jwhhh