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

overhaul of input-output functionality #87

Merged
merged 11 commits into from
Sep 28, 2021
Merged

overhaul of input-output functionality #87

merged 11 commits into from
Sep 28, 2021

Conversation

guyvdbroeck
Copy link
Member

  • replaced LoadSave module by io directory
  • removed logistic circuit loaders
  • rewrite of .psdd parser using Lerche.jl
  • use parse, read and write from Base to do IO, no custom function names
  • add our own flexible file format .jpc which can be read as either ProbCircuit or StructProbCircuit
  • allow read/write in combination with a .vtree file for IO of StructProbCircuit

I used the occasion for some general cleanup:

  • got rid of all things LogisticCircuit, regression circuit, circuit pairs, etc., these should go into their own package and receive more dedicated attention
  • removed some tests that were too slow

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2021

Codecov Report

Merging #87 (1061b7d) into master (d456ce6) will increase coverage by 0.96%.
The diff coverage is 72.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   69.05%   70.02%   +0.96%     
==========================================
  Files          45       37       -8     
  Lines        3490     2802     -688     
==========================================
- Hits         2410     1962     -448     
+ Misses       1080      840     -240     
Impacted Files Coverage Δ
src/ProbabilisticCircuits.jl 100.00% <ø> (ø)
src/factor_graph/factor_graph.jl 100.00% <ø> (ø)
src/factor_graph/fg_compile.jl 56.41% <ø> (ø)
src/io/ensemble_io.jl 0.00% <0.00%> (ø)
src/io/plot.jl 0.00% <ø> (ø)
src/param_bit_circuit.jl 93.10% <ø> (-1.64%) ⬇️
src/parameters.jl 61.22% <0.00%> (ø)
src/io/clt_io.jl 73.68% <73.68%> (ø)
src/io/io.jl 88.88% <88.88%> (ø)
src/io/jpc_io.jl 93.15% <93.15%> (ø)
... and 16 more

Continue to review full report at Codecov.

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

Copy link
Contributor

@khosravipasha khosravipasha left a comment

Choose a reason for hiding this comment

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

Overall the IO changes seem good to go.

For the removal of logistic/expectation stuff, can we wait to remote them later? I think its easier to fix/complete them inside PCs and then move them to new package when ready (cause there is plenty overhead with setting up a new package)

I will also add the removed/commented slow tests later to slow test action we have (#88)

@guyvdbroeck
Copy link
Member Author

I agree it would be useful to already move the commented-out slow tests to the slow test functionality.

For the logistic circuits, it didn't make sense to wait because the logistic circuit parser was removed from LogicCircuits.jl, and without it the expectations etc. don't work anyway. Better bite the bullet, and check out the older commit when making LogisticCircuits.jl in future. Also, who is maintaining the logistic circuit functionality? In the past we have never had that stuff functioning in Juice.

@khosravipasha
Copy link
Contributor

Sounds good, we can merge this then. I can make the new package. I only wrote the parser to read logistic circuits. I think yitao wrote the paramter learning, we don't support anything else yet for logistic/regression circuits.

@khosravipasha khosravipasha merged commit fea596e into master Sep 28, 2021
@khosravipasha khosravipasha deleted the parser branch November 29, 2021 21:07
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.

3 participants