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

Use matlab_round instead of round to fix precompilation issues #16

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

Tortar
Copy link
Collaborator

@Tortar Tortar commented Oct 17, 2024

Precompilation failed before this:

julia> using BeforeIT
Precompiling BeforeIT...
Info Given BeforeIT was explicitly requested, output will be shown live 
WARNING: Method definition round(Any) in module Base at rounding.jl:472 overwritten in module BeforeIT at /home/bob/.julia/dev/BeforeIT/src/utils/positive.jl:80.
ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.
  ? BeforeIT
[ Info: Precompiling BeforeIT [ca9fcad7-41d0-4f76-b1e5-366c28bce52e] 
WARNING: Method definition round(Any) in module Base at rounding.jl:472 overwritten in module BeforeIT at /home/bob/.julia/dev/BeforeIT/src/utils/positive.jl:80.
ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.
┌ Info: Skipping precompilation due to precompilable error. Importing BeforeIT [ca9fcad7-41d0-4f76-b1e5-366c28bce52e].
└   exception = Error when precompiling module, potentially caused by a __precompile__(false) declaration in the module.

p.s. Very Cool Project!!!

@AldoGl
Copy link
Collaborator

AldoGl commented Oct 21, 2024

Thank you @Tortar for your fix! This seems correct to me. @Devetak what do you think?

By the way, that part of the code is highly experimental and untested. If using it you end up improving quality/modularity please don't hesitate to push further changes :)

@Devetak
Copy link
Collaborator

Devetak commented Oct 21, 2024

Looks good, thank you for fixing it. I think we can squash and merge.

Btw @Tortar any insight why we never spotted this error? Do you have any special flags?

@Tortar
Copy link
Collaborator Author

Tortar commented Oct 21, 2024

I checked now, seems like this warning is present only in 1.11, which is kind of strange, at the same time this is piracy so it should be good to fix it

@AldoGl
Copy link
Collaborator

AldoGl commented Oct 21, 2024

Perfect guys, I am rebasing and merging now!

@AldoGl AldoGl merged commit 664a561 into bancaditalia:main Oct 21, 2024
2 checks passed
@AldoGl
Copy link
Collaborator

AldoGl commented Oct 22, 2024

Thanks @Tortar, I added you to the list of contributors. Once again, don't hesitate to push more changes.

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