search

SciML / JumpProcesses.jl

Build and simulate jump equations like Gillespie simulations and jump diffusions with constant and state-dependent rates and mix with differential equations and scientific machine learning (SciML)
https://docs.sciml.ai/JumpProcesses/stable/
Other
135 stars 35 forks source link

Refactor internal functions in spatial SSAs. #342

Closed Vilin97 closed 10 months ago

Vilin97 commented 10 months ago

This is a pure refactoring PR to make adding support for spatial constant rate jumps easier.

Vilin97 commented 10 months ago

Hmm, I am confused about the build error. It tells me that some files are not formatted, so I ran the formatter in VSCode. But that did not remove the build error. How should I run the formatter to get rid of the build error? @isaacsas

In case it helps the review, the first commit has all the real changes. The second commit is the result of the format command.

The command I ran: image

isaacsas commented 10 months ago

Don't worry about the formatting. There was a change that was terrible and made a really bad formatting alteration that messes up files in many people's opinion. The proposed fix got bogged down in bikeshedding and so nothing has been implemented yet. For just ignore formatting warnings, and don't try to run the formatter yourself. (If you really feel you need to format, stick with version 1.0.32 of JuliaFormatter.)

isaacsas commented 10 months ago

Please revert any formatter created changes please.

Vilin97 commented 10 months ago

@isaacsas , I reverted the formatting. What is the timeline for fixing the formatting issue? It's scary to see the red x everywhere because it makes it feel like our CI is broken, which makes it feel like the code health of the package is bad.

isaacsas commented 10 months ago

The timeline is for someone to make an executive decision on the formatting change to switch to, and then for someone to update the style at JuliaFormatter. I've pinged the thread once already suggesting we use the option that had the most support and least pushback to get this resolved, but that didn't seem to get traction, so there isn't much more I can do.

If you look at the individual tests we aren't merging anything that fails CI. It is just the formatter action that is failing. I'd happily remove it until this is settled, but I think @ChrisRackauckas wants to keep it on all SciML libraries as policy. If it really bothers you format with the version I mentioned and the test should then pass. (Or format with that version in a separate PR, and then update this once that is merged to avoid polluting the diff here.)