Skip to content

chore: update arpack to arpack-ng 3.9.1 #1559

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

szhorvat
Copy link
Member

@szhorvat szhorvat commented Oct 21, 2024

@krlmlr You asked to update ARPACK to the latest ARPACK-NG. This PR attempts to do that. It was quite painful: first we need to rename functions to have an igraph prefix, and avoid conflicts. This can be done automatically. But then the line lengths exceed 72 characters, which is the maximum for fixed-form Fortran, and we need to manually reformat them, with the appropriate continuation character. I hope I didn't mess this up.

The changes to snapshots are fine. But there's a failure with spectral clustering, and it's unclear to me whether that's a real failure or not. I'll have another look at this when teaching is over (mid-November).

Copy link
Contributor

aviator-app bot commented Oct 21, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@szhorvat
Copy link
Member Author

@krlmlr I am quite certain that this is better than the old version, despite the test failures. That said, the tests need to be dealt with carefully before going ahead, and I won't have time for this before 2.2.0.

However, running revdepchecks for this would be quite useful, to test if anything in the real world was actually broken by this change. I'm not sure if that's possible before fixing our tests @krlmlr .

szhorvat and others added 2 commits March 30, 2025 14:05
Co-authored-by: szhorvat <1212871+szhorvat@users.noreply.github.com>
@szhorvat szhorvat force-pushed the chore/arpack-3.9.1 branch from 7062d99 to ba254aa Compare March 30, 2025 14:05
@krlmlr
Copy link
Contributor

krlmlr commented Apr 10, 2025

Running revdepchecks now.

@krlmlr
Copy link
Contributor

krlmlr commented Apr 10, 2025

The results don't look terrible, comparing with the main branch now.

@krlmlr
Copy link
Contributor

krlmlr commented Apr 10, 2025

Are you okay with merging this, assuming we can fix the tests?

@schochastics
Copy link
Contributor

schochastics commented Apr 10, 2025

Are you okay with merging this, assuming we can fix the tests?

Just checked locally. The error occurs at random. Need to have a closer look , but I think it should be fine to merge?

@szhorvat
Copy link
Member Author

I think it's best if I have a look at this first, and see if the parameters with which ARPACK is called need to be tuned. I'll also need to repro the issue in the C core.

Let's delay merging this to the next release, or maybe I can get to it on the weekend.

Thanks for running the revdepchecks, this was very useful! On a first skim, I see nothing ARPACK-related, which is indeed good.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants