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

Some (many?) proposals #56

Closed
2 of 7 tasks
ghost opened this issue Jul 3, 2018 · 7 comments
Closed
2 of 7 tasks

Some (many?) proposals #56

ghost opened this issue Jul 3, 2018 · 7 comments

Comments

@ghost
Copy link

ghost commented Jul 3, 2018

Hello, newbie here. I have in mind some features and possible optimization that
could help to improve this package, also in terms of maintainability. I wanted
to open this issue for having a feedback from the maintainers before starting
to open Pull Requests.

My ideas consist of

  • Splitting source files into something like "utilities.jl", "types.jl",
    "basics.jl", "operations.jl", "specials.jl". I think that each file name
    is self-explicative.
  • Making functions for AbstractLinearOperator more generic, substituting
    explicit fields invocations (e.g. op.symmetric) with traits (e.g.
    issymmetric(op)).
  • Making LinearOperator immutable (don't have find any function nor
    reason why it should be mutable but, please, correct me if I am wrong).
  • Parametrizing LinearOperator{T} with the type of transformation
    function, (to become LinearOperator{T, Fprod, Ftprod, Fctprod}) so that
    there is more specialization and in theory a better performance on code
    (this has to be tested).
  • Using undef to signal non-specified tprods and ctprods instead
    of nothing, because in my opinion it would be more self-describing.
  • Creating a new struct to replace v -> M * v, something like a callable
    struct MatrixProduct{T <: AbstractMatrix}
        M::T
    end
    (mp::MatrixProduct)(v) = mp.M * v
    to preserve information about M and use it to combine linear operators in
    a more efficient manner.
  • Removing information about being symmetric and being hermitian from
    fields and creating two new AbstractLinearOperators such as
    SymmetricLinearOperator and HermitianLinearOperator with which to
    dispatch, instead of using many if-else statements. Symmetry can then
    be checked as a trait and it could be useful a function linearoperator(x)
    that transform a Matrix or a function in the most informative type.

I am ready to take care of these PRs. None of them seems breaking to me,
but first of all let me know what do you think about them.

@abelsiqueira
Copy link
Member

Hello @gpapo, thanks for the proposals, they seem to really improve the code. @dpo had a few modifications regarding type stability, so let's wait for his opinion.

@abelsiqueira
Copy link
Member

@dpo Any opinions here?

@dpo
Copy link
Member

dpo commented Aug 8, 2018

Working on it.

@dpo
Copy link
Member

dpo commented Sep 5, 2018

#59 introduces types for internal functions (prod, tprod, ctprod). The main goal is type stability, but we're not there yet and there are some issues.

@gpapo All other contributions are very welcome. Thanks!

@dpo
Copy link
Member

dpo commented Sep 19, 2018

@gpapo Some thoughts on your proposals:

Splitting source files

Good idea.

Making functions for AbstractLinearOperator more generic, substituting explicit fields invocations (e.g. op.symmetric) with traits (e.g. issymmetric(op)).

Sounds good to me.

Making LinearOperator immutable (don't have find any function nor reason why it should be mutable but, please, correct me if I am wrong).

I think you're right. Is there a performance advantage if they are immutable, or is the advantage mainly to protect against accidental changes?

Parametrizing LinearOperator{T} with the type of transformation function, (to become LinearOperator{T, Fprod, Ftprod, Fctprod})

I think that's essentially what's going on in #59. Some operations are still type unstable though.

Using undef to signal non-specified tprods and ctprods instead of nothing, because in my opinion it would be more self-describing.

I don't have a strong opinion on this. Is there an advantage?

Creating a new struct to replace v -> M * v

Could you give elaborate on how this could be used to combine operators more efficiently?

creating two new AbstractLinearOperators such as SymmetricLinearOperator and HermitianLinearOperator

So essentially, the user would "promise" that the operator defined is symmetric/hermitian? I presume we could run the quick checks but we couldn't possibly assert that they are truly symmetric/hermitian.

@ghost
Copy link
Author

ghost commented Sep 19, 2018

Could you give elaborate on how this could be used to combine operators more efficiently?

For example one could say that linear operator M ∘ N is M * N and not x -> M ⋅ (x -> N ⋅ x) and could not need FastClosures.
I will try to work on some of these proposals and do some benchmarks, even if in this period I am quite busy with my lectures.

This was referenced Nov 11, 2019
@abelsiqueira
Copy link
Member

I've created two simpler issues related to the proposal here: #113 and #114. Some are already done, and some have been done and undone (like the types of prod, tprod and ctprod). The other issues have no clear advantage, so they could be discussed further in the future. Hence I'm closing this issue.

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

No branches or pull requests

2 participants