Skip to content

bmti: add faster cg-based solver, remove old solvers #149

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

Closed
wants to merge 1 commit into from

Conversation

AldoGl
Copy link
Collaborator

@AldoGl AldoGl commented Jan 7, 2025

Proposed changes

I introduce a much more efficient solver for the solution of the BMTI linear system. The new solver makes the overall algorithm very memory- and time-efficient and scales roughly linearly with the number of points.

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

@AldoGl
Copy link
Collaborator Author

AldoGl commented Jan 7, 2025

The new solver is much faster than the old ones and is also very memory efficient, hence we don't need a "memory_efficient" flag anymore as we can have it efficient by default

@AldoGl AldoGl changed the title bmti: add faster cg-based solver, remove old solvers [WIP] bmti: add faster cg-based solver, remove old solvers Jan 7, 2025
@AldoGl AldoGl requested a review from charliematteo January 7, 2025 20:09
@AldoGl
Copy link
Collaborator Author

AldoGl commented Jan 7, 2025

As you can see from the modified test, the new estimates are within rtol=1e-5 and atol=1e-1 from the old ones. I think we can live with it!

@AldoGl AldoGl changed the title [WIP] bmti: add faster cg-based solver, remove old solvers bmti: add faster cg-based solver, remove old solvers Jan 7, 2025
@giovannidoni
Copy link

Questo repositiry implementa cose molto carine, che sospetto (sto già verificano) possano essere molto utili. Incoraggio il development e se serve posso dare una mano

@charliematteo
Copy link
Collaborator

Questo repositiry implementa cose molto carine, che sospetto (sto già verificano) possano essere molto utili. Incoraggio il development e se serve posso dare una mano

Ciao @giovannidoni, grazie mille! Ti riferisci a questo specifico branch o a DADApy in generale? Se ti interessa BMTI, la versione più aggiornata è sul branch bmti_solvers, che verrà mergiata al main molto presto.

Ad ogni modo, abbiamo molte direzioni di sviluppo in testa. Se ti interessa qualcosa in particolare facci sapere, possiamo pensarci assieme e/o dare priorità a quella

@giovannidoni
Copy link

Per prima cosa sottolineo la rilevanza in ambito commerciale. Oggi ho provato per la prima volta e ho notato che si blocca su density estimation ma nn sono riuscito a guardarci dentro. L'altra pr recente su more scalable algo sembra utile. Sto considerando di usare ADP per un lavoro, data la perf che ho visto, quindi penso che sia prioritario rendere l'implementazione corrente air tight.

@giovannidoni
Copy link

giovannidoni commented Mar 27, 2025

FYI, scrivo qui perchè è l'unico modo che ho di comm cn i maintainers. io ho cominciato da poco a portare Pamm in python https://github.com/giovannidoni/pypamm

@charliematteo
Copy link
Collaborator

Grazie mille dei feedback. Density estimation dici che si blocca nel senso che crasha o nel senso che rallenta, cioè c'è un bottleneck computazionale?

@giovannidoni
Copy link

giovannidoni commented Mar 27, 2025

non saprei, domani ci guardo ma si blocca su density estimation step; non credo sia un bottleneck dato la size del sys, ma a naso un while loop. Altra cosa che ho notato e' una exception su una density non finita qui

assert not np.isnan(np.sum(self.log_den)), "log density contains nan values"

@charliematteo
Copy link
Collaborator

Allora ho due domande:

  • che branch di DADApy stai usando? Il main o altri?
  • che density estimator stai usando? Quando chiami data.compute_density_...()

In risposta alla tua domanda quella linea è un assert che controlla che tutte le logdensities e tutti gli errori sulle logdensities siano dei numeri e non siano NAN. Siccome il clustering si basa su queste due cose, se ci sono dei NAN negli array, quell'assert blocca tutto e dà un errore.

@giovannidoni
Copy link

giovannidoni commented Mar 27, 2025

che branch di DADApy stai usando? Il main o altri?

main (da pipy)

che density estimator stai usando? Quando chiami data.compute_density_...()

.compute_clustering_ADP(...)

a quella linea è un assert che controlla che tutte le logdensities e tutti gli errori sulle logdensities siano dei numeri e non siano NAN.

si, lo so, la domanda e' come mai. Varie ragioni immagino, ragionevoli, peraltro ma quale sarebbe il un fallback sensato?

@charliematteo
Copy link
Collaborator

.compute_clustering_ADP(...)

Prima di compute clustering però devi fare un compute density. Come si chiama quello che usi? PAk? kstarNN?

si, lo so, la domanda e' come mai. Varie ragioni immagino, ragionevoli, peraltro ma quale sarebbe il un fallback sensato?

Beh, la ragione è di "debugging", di controllo. Se le densità e gli errori hanno dei NAN devi capire il perché debuggando più che risolvere a run time. Anche perché di base compute_clustering_ADP(...) funziona a prescindere da come le densità e gli errori sono stati calcolati, non serve averli calcolati in DADApy.

@giovannidoni
Copy link

Nel caso specifico ho usato 2NN per la densita'

@charliematteo
Copy link
Collaborator

Nel caso specifico ho usato 2NN per la densita'

2NN è per la dimensione intrinseca. Che succede prima della densità. Se tra 2NN e ADP non hai chiamato nessun compute_density_... allora hai usato il default che è PAk.
E comunque non ho ancora capito se sulla density rallenta molto e si "impalla" senza mai uscire dal processo o proprio crasha

@giovannidoni
Copy link

giovannidoni commented Mar 27, 2025

No non va in crash, non ritorna, da cui l'ipotesi che sia un while loop e quello e' un primo problema che ho riscontrato; il secondo e' quello dei nans sulla densita. Ci riguardo e ti faccio sap che a mente potrei non essere troppo preciso

@charliematteo
Copy link
Collaborator

@giovannidoni se vuoi apri un issue su questo fatto che la densità si pianta. Che ne discutiamo lì. Potrebbe essere un effetto della dimensionalità dei tuoi dati, ma vediamo

@AldoGl
Copy link
Collaborator Author

AldoGl commented Apr 11, 2025

This development has been brought to #154, let's discuss it there!

@AldoGl AldoGl closed this Apr 11, 2025
@AldoGl AldoGl mentioned this pull request Apr 11, 2025
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