Add Side Length checker

exAClior commented 1 year ago

I added checks for side length to be greater than zero for CubicBoundary and RectangularBoundary.

I did two unit tests locally. One of them gave an error shown below. Looks like zygote might create a CubicBoundary with negative side length. However, I was not able to reproduce this again.

Gradients: Error During Test at /Users/yushengzhao/projects/Molly.jl/test/zygote.jl:1
  Got exception outside of a @test
  AssertionError: Side lengths need to be larger than 0
  Stacktrace:
    [1] CubicBoundary
      @ ~/projects/Molly.jl/src/spatial.jl:38 [inlined]
    [2] accum(x::NamedTuple{(:side_lengths,), Tuple{SizedVector{3, Float64, Vector{Float64}}}}, y::SVector{3, Float64})
      @ Molly ~/projects/Molly.jl/src/zygote.jl:137
    [3] (::Zygote.var"#back#291"{:contents, Zygote.Context{false}, Core.Box, CubicBoundary{Float64}})(Δ::SVector{3, Float64})
      @ Zygote ~/.julia/packages/Zygote/xGkZ5/src/lib/lib.jl:237
    [4] (::Zygote.var"#2149#back#292"{Zygote.var"#back#291"{:contents, Zygote.Context{false}, Core.Box, CubicBoundary{Float64}}})(Δ::SVector{3, Float64})
      @ Zygote ~/.julia/packages/ZygoteRules/OgCVT/src/adjoint.jl:71
    [5] Pullback
      @ ~/projects/Molly.jl/test/zygote.jl:136 [inlined]
    [6] (::typeof(∂(λ)))(Δ::Float64)
      @ Zygote ~/.julia/packages/Zygote/xGkZ5/src/compiler/interface2.jl:0
    [7] (::Zygote.var"#68#69"{typeof(∂(λ))})(Δ::Float64)
      @ Zygote ~/.julia/packages/Zygote/xGkZ5/src/compiler/interface.jl:45
    [8] gradient(::Function, ::Float64, ::Vararg{Float64})
      @ Zygote ~/.julia/packages/Zygote/xGkZ5/src/compiler/interface.jl:97
    [9] macro expansion
      @ ~/projects/Molly.jl/test/zygote.jl:196 [inlined]
   [10] macro expansion
      @ /Applications/Julia-1.8.app/Contents/Resources/julia/share/julia/stdlib/v1.8/Test/src/Test.jl:1363 [inlined]
   [11] top-level scope
      @ ~/projects/Molly.jl/test/zygote.jl:2
   [12] include(fname::String)
      @ Base.MainInclude ./client.jl:476
   [13] top-level scope
      @ ~/projects/Molly.jl/test/runtests.jl:82
   [14] include(fname::String)
      @ Base.MainInclude ./client.jl:476
   [15] top-level scope
      @ none:6
   [16] eval
      @ ./boot.jl:368 [inlined]
   [17] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:276
   [18] _start()
      @ Base ./client.jl:522
Test Summary: | Pass  Error  Total     Time
Gradients     |    9      1     10  1m41.3s
ERROR: LoadError: Some tests did not pass: 9 passed, 0 failed, 1 errored, 0 broken.
in expression starting at /Users/yushengzhao/projects/Molly.jl/test/zygote.jl:1
in expression starting at /Users/yushengzhao/projects/Molly.jl/test/runtests.jl:81
ERROR: Package Molly errored during testing

JaydevSR commented 1 year ago

Shouldn't this be a DomainError? It seems more appropriate for the cases when only certain set of arguments are allowed. Also, array construction (i.e. [x, y, z] etc.) can also be easily skipped in the condition by using &&s instead, not that it would make a big difference but still 🤔

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (6de8b96) 83.77% compared to head (745e439) 83.78%.

:exclamation: Current head 745e439 differs from pull request most recent head 0240b25. Consider uploading reports for the commit 0240b25 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #120 +/- ## ======================================= Coverage 83.77% 83.78% ======================================= Files 33 33 Lines 3969 3971 +2 ======================================= + Hits 3325 3327 +2 Misses 644 644 ``` | [Impacted Files](https://codecov.io/gh/JuliaMolSim/Molly.jl/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMolSim) | Coverage Δ | | |---|---|---| | [src/spatial.jl](https://codecov.io/gh/JuliaMolSim/Molly.jl/pull/120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMolSim#diff-c3JjL3NwYXRpYWwuamw=) | `75.59% <100.00%> (+0.23%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMolSim). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMolSim)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

exAClior commented 1 year ago

Shouldn't this be a DomainError? It seems more appropriate for the cases when only certain set of arguments are allowed. Also, array construction (i.e. [x, y, z] etc.) can also be easily skipped in the condition by using &&s instead, not that it would make a big difference but still 🤔

You are absolutely correct about both. Thank you so much for pointing them out! I considered other ways instead of doing [x,y,z] but nothing gives a clean-looking code since I would need to include the unit(x) for all three sides. Or maybe I am misinterpreting your suggestion of using &&?

JaydevSR commented 1 year ago

Shouldn't this be a DomainError? It seems more appropriate for the cases when only certain set of arguments are allowed. Also, array construction (i.e. [x, y, z] etc.) can also be easily skipped in the condition by using &&s instead, not that it would make a big difference but still 🤔

You are absolutely correct about both. Thank you so much for pointing them out! I considered other ways instead of doing [x,y,z] but nothing gives a clean-looking code since I would need to include the unit(x) for all three sides. Or maybe I am misinterpreting your suggestion of using &&?

This is not an issue here infact it will make no difference at all as this is just setup code for a simulation. But usually we try to avoid array allocation in simulation codes so I just pointing it out out of a habit 😅. So you can easily ignore the second comment.

exAClior commented 1 year ago

Thanks for doing this.

Indeed it could be a DomainError. Also, the check can be made once as part of an inner constructor in the struct definition.

With regards to the error, I just remembered that the gradient code uses boundaries with zero length sides to accumulate gradients. So maybe just check that side lengths aren't negative to allow this to work.

Thank you so much for pointing out the inner constructor, I think it solves some of the concerns about using [x,y,z] Hopefully, the code is now a bit neater.

I also allowed negative values to pass too.

JuliaMolSim / Molly.jl

Add Side Length checker #120

Codecov Report