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

Drop deprecated LAPACK apis #2857

Merged
merged 8 commits into from
Jun 19, 2019
Merged

Conversation

jschueller
Copy link
Contributor

dgelsx, dgegv, dgeqpf are deprecated since lapack 3.1.1 (2007), remove them

@beutlich beutlich added the L: Math Issue addresses Modelica.Math label Mar 19, 2019
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

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

Thanks for reporting and fixing this. We are in the preparations for new major MSL (where we are actually allowed to break backwards-compatibility).

As these functions are not used within the MSL it seems sufficient. However, instead of removing these four functions, we require to

  • move them to a (to be created) package ObsoleteModelica4.Math.Matrices.LAPACK,
  • provide conversion scripts for automatic updating of user libraries (by either using the moved functions or the replacement functions dgelsy, dggev, dgeqp3)
  • test models to test the conversion.

Saying this, we missed to mark them as obsolete now, such that users of the latest MSL v3.2.3 are aware of the deprecated functions and the change does not come as unexpected surprise. Not sure, how strict we should be here.

@jschueller
Copy link
Contributor Author

I dont know how to create packages, can you provide details ?

@beutlich beutlich self-assigned this Apr 3, 2019
dgelsx, dgegv, dgeqpf are deprecated since lapack 3.1.1 (2007), remove them
@beutlich beutlich added this to the MSL4.0.0 milestone Jun 13, 2019
@beutlich beutlich added the task General work that is not related to a bug or feature label Jun 13, 2019
@beutlich
Copy link
Member

@HansOlsson @MartinOtter This PR is now ready for your review.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

The work-areas for dgeqp3 does not seem ideal since:

  • It is the minimum possible; which will not utilize the level-3 block algorithm in a good way. Would it be possible to have lwork as an optional input (with suitable default)?
  • The work-area length is duplicated. If we cannot use work[lwork] would it be possible to not have "lwork" as a variable and instead replace lwork in the argument list by size(work,1)?

@beutlich
Copy link
Member

The work-areas for dgeqp3 does not seem ideal since:

* It is the minimum possible; which will not utilize the level-3 block algorithm in a good way. Would it be possible to have lwork as an optional input (with suitable default)?

Hm, could do so. What would be a suitable default for lwork then? Leave it as max(1, 3*size(A, 2) + 1)?

* The work-area length is duplicated. If we cannot use work[lwork] would it be possible to not have "lwork" as a variable and instead replace lwork in the argument list by size(work,1)?

Many LAPACK functions do so (for example dgesvd_sigma) and I just adopted this style of implementation.

@HansOlsson
Copy link
Contributor

Hm, could do so. What would be a suitable default for lwork then? Leave it as max(1, 3*size(A, 2) + 1)?

Yes.

Many LAPACK functions do so (for example dgesvd_sigma) and I just adopted this style of implementation.

You're right. That should likely be improved for more functions.
However, it is related to the change in this PR, since level-3 routines need larger work-areas and we here switch to a level-3 implementation.

@beutlich
Copy link
Member

beutlich commented Jun 18, 2019

* The work-area length is duplicated. If we cannot use work[lwork] would it be possible to not have "lwork" as a variable and instead replace lwork in the argument list by size(work,1)?

Note, that a an Integer variable lwork is mapped to int type in C, but size(work, 1) would map to size_t in C, but impossible in Fortran. See also #1774.

@beutlich beutlich requested a review from HansOlsson June 19, 2019 09:20
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Looks ok.

@beutlich beutlich added the CI Issue that addresses continuous integration label Jun 19, 2019
@beutlich beutlich removed the request for review from MartinOtter June 19, 2019 09:58
@beutlich beutlich merged commit fe4e4a0 into modelica:master Jun 19, 2019
@jschueller jschueller deleted the lapack_deprecated branch September 10, 2019 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issue that addresses continuous integration L: Math Issue addresses Modelica.Math task General work that is not related to a bug or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants