Skip to content

Conversation

@tpapp
Copy link
Collaborator

@tpapp tpapp commented Jul 9, 2025

Introduces a separate test environment to reproduce the problem in #94 (which is unrelated to that PR), then adds a fix by rearranging the branches and testing for infinites early.

I still don't understand why the previous setup was not using the latest ForwardDiff.jl, but this one does.

This was referenced Jul 9, 2025
@tpapp tpapp requested a review from devmotion July 9, 2025 13:19
@oscardssmith
Copy link
Contributor

Can you revert the test Project changes? IMO the [extras] way of managing versions works better (and TestEnv.jl can be used to activate the test environment), since the separate Project.toml approach is easy to have issues where someone updates one but not the other, leading to issues.

@tpapp
Copy link
Collaborator Author

tpapp commented Jul 9, 2025

I don't agree, I think that with the [extras] you can easily run into mystery issues like not catching this and using an ancient ForwardDiff.jl. But I am open to arguments and you can convince me to revert that part if necessary.

@oscardssmith
Copy link
Contributor

I don't especially care. I think this should merge.

@devmotion
Copy link
Member

I think the correct fix for mysterious test environments would be to add compat entries for test environments. This is what a currently missing Aqua test would require as well.

@oscardssmith
Copy link
Contributor

yeah compats for test env is a good idea in general.

@tpapp
Copy link
Collaborator Author

tpapp commented Jul 14, 2025

I added QA with Aqua and JET. I am merging this as is, so that #94 can proceed, if this causes issues we can go back to having test packages in the main Project.toml.

@tpapp tpapp merged commit c9adf5a into master Jul 14, 2025
4 checks passed
@tpapp tpapp deleted the tp/test-environment branch July 14, 2025 08:10
real_x = real(x)
real_xmax = real(xmax)
if real_x > real_xmax
if isinf(real_x) && isinf(real_xmax) && (real_x * real_xmax) > 0
Copy link
Member

Choose a reason for hiding this comment

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

I think this change should be benchmarked. IIRC the performance of logsumexp was very sensitive to even minor changes of the implementation. To avoid the multiplication one might want to use

Suggested change
if isinf(real_x) && isinf(real_xmax) && (real_x * real_xmax) > 0
if isinf(real_x) && isinf(real_xmax) && sign(real_x) == sign(real_xmax)

Comment on lines +1 to +13
[deps]
Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4"
ChainRulesTestUtils = "cdddcdb0-9152-4a09-a978-84456f9df70a"
ChangesOfVariables = "9e997f8a-9a97-42d5-a9f1-ce6bfc15e2c0"
FiniteDifferences = "26cc04aa-876d-5657-8c51-4c34ba976000"
ForwardDiff = "f6369f11-7733-5829-9624-2563aa707210"
InverseFunctions = "3587e190-3f89-42d0-90ee-14403ec27112"
JET = "c3a54625-cd67-489e-a8e7-0a5a0ff4e31b"
LogExpFunctions = "2ab3a3ac-af41-5b50-aa03-7779005ae688"
OffsetArrays = "6fe1bfb0-de20-5000-8ca7-80f57d26f881"
Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
Copy link
Member

Choose a reason for hiding this comment

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

IMO test dependencies should also be specified with compat entries and this file should be updated with CompatHelper.

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