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

add a new macro parameter ho, in order to reuse existing ho. #45

Merged
merged 7 commits into from
Feb 18, 2021

Conversation

norci
Copy link
Contributor

@norci norci commented Feb 15, 2021

so we can save ho progress periodically. or resume ho.

fixes #23

so we can save ho progress periodically. or resume ho.
@baggepinnen
Copy link
Owner

Cool thanks for taking a stab at this!
It does not appear like the new functionality is tested? There should ideally be a tests for each unique sampler as they all behave slightly differently, and some test that the number of iterations is correctly updated etc.

@norci
Copy link
Contributor Author

norci commented Feb 16, 2021

I have added some test cases.
I also refactored the code, removed a few duplicate code. I'm not good at meta programming. wish these changes won't break anything.

for it might contain variables captured from outside.
if these variables have changed, then the old ho will be invalid.
so should only resume sampler and history.
@baggepinnen
Copy link
Owner

Cool! Are you happy with the PR and it's ready to be merged?

@norci
Copy link
Contributor Author

norci commented Feb 18, 2021

Yes. I have used it for a whole day. seems OK.
Please merge it.
Thanks.

@baggepinnen baggepinnen merged commit fbeb734 into baggepinnen:master Feb 18, 2021
@baggepinnen
Copy link
Owner

Awesome, thanks!
A new version will be released in an hour or so :)

@baggepinnen
Copy link
Owner

baggepinnen commented Mar 14, 2021

It seems this might actually have broken the parallel @phyperopt #48 . I noticed that you removed the workaround_function that was previously used to get @phyperopt working, can you remember the reasoning for this?

The tests had by mistake not tested parallel execution since nworkers() returns 1 if no additional processes have been started`

@norci
Copy link
Contributor Author

norci commented Mar 14, 2021

It seems this might actually have broken the parallel @phyperopt #48 . I noticed that you removed the workaround_function that was previously used to get @phyperopt working, can you remember the reasoning for this?

I'm sorry for that.
The only reason is remove redundant code. Unfortunately this mistake was not covered by the test cases :(

I just know Pkg test has a parameter "--coverage" last week.
This is very helpful.
Is there a way to generate a test coverage report locally?

@baggepinnen
Copy link
Owner

No worries, the tests certainly appeared to cover also the parallel case. The problem is solved by now. If you want to produce coverage files locally, I believe you can have coverage results printed to file with the second option below

 --code-coverage={none|user|all}, --code-coverage
                           Count executions of source lines (omitting setting is equivalent to "user")
 --code-coverage=tracefile.info
                           Append coverage information to the LCOV tracefile (filename supports format tokens).

There are also some tools for this available at https://github.com/JuliaCI/Coverage.jl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable warm start
2 participants