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

Default construction argument for Matrix class #48

Open
vetlewi opened this issue Oct 8, 2019 · 2 comments
Open

Default construction argument for Matrix class #48

vetlewi opened this issue Oct 8, 2019 · 2 comments
Labels
feature_request Requestion this feature

Comments

@vetlewi
Copy link
Collaborator

vetlewi commented Oct 8, 2019

If a Matrix object is constructed with a single argument, the argument is currently assumes this argument to be an array containing the matrix. However, I think that the most common way to use the matrix class will not to pass a Numpy array, but to pass the path to the file. It would therefore be more intuitive if one could call the constructor with
mat = ompy.matrix("raw_spectra.m") rather than
mat = ompy.matrix(path="raw_spectra.m")

One way to solve this could be that if there is only one argument, then the constructor checks the type of the argument, and based on if's a string or a Numpy array it proceeds with the appropriate steps.

I suggest that one could do such a check for this with something like:

if Eg is None and Ex is None and std is None and path is None and shape is None and state is None and isinstance(values, str):
      self.load(path)
@vetlewi vetlewi added the feature_request Requestion this feature label Oct 8, 2019
@ErlendLima
Copy link
Contributor

The downside is that the initialization signature would be quite complicated. A possible solution is to only allow the values/path variable to be set by position while forcing all others to be set by keyword.

To illustrate the problem, it is easy to guess what mat = om.Matrix(values, Eg, Ex) will result in. But what about mat = om.Matrix('raw.m', Eg). Should the Eg array overwrite the loaded array? Should it throw an error? If the former, then mat = om.Matrix('raw.m', values=values) ought to be allowed; load the energy vectors but overwrite the matrix with new values.

Forcing keywords have the other advantage of removing bugs of the type mat = om.Matrix(values, Ex, Eg), since with keywords we get the obviously correct mat = om.Matrix(values, Ex=Ex, Eg=Eg).

@fzeiser
Copy link
Collaborator

fzeiser commented Oct 9, 2019

I agree with Erlend. In the code, we quite often create some (new) matrices using the type mat = om.Matrix(values, Ex, Eg). So I think it might be nice to have following signatures:

  • mat = om.Matrix('raw.m') -> load path
  • mat = om.Matrix(values, Ex=..., Eg=...) --> load matrix
  • mat = om.Matrix('raw.m', Eg) --> give an error
  • mat = om.Matrix('raw.m', values=values) --> give an error
  • mat = om.Matrix(values) --> is this really relevant / potentially again give an error

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

No branches or pull requests

3 participants