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

Input argument parsing #80

Open
jeanluct opened this issue Nov 27, 2014 · 10 comments
Open

Input argument parsing #80

jeanluct opened this issue Nov 27, 2014 · 10 comments
Assignees
Milestone

Comments

@jeanluct
Copy link
Owner

(From Marko)

As use-cases for different functions, e.g., entropy.m, it becomes more and more cumbersome to manually check and update their argument list. We should convert these more complicated functions to use inputParser.

@jeanluct jeanluct added the new label Nov 27, 2014
@jeanluct
Copy link
Owner Author

From Jean-Luc Thiffeault on 2014-10-22 20:07:09+00:00

Absolutely. The current setup is a mess.

@jeanluct
Copy link
Owner Author

From Marko Budisic on 2014-10-22 20:45:50+00:00

entropy.m now supports different length functions; slight change in default arguments - tol = [] now implies tol = default, not tol = 0 as before. This was done to allow skipping it. For a long-term solution see issue #80

→ <<cset 327ddecf0e91>>

@jeanluct
Copy link
Owner Author

From Marko Budisic on 2014-10-30 23:05:39+00:00

I have just moved entropy to inputParser ( ea5ea27 ). This was my first approximation to how things could look like. 2b13736 introduced a function that can be used to specify multiple names for a single flag, e.g. "bh", "trains", etc. for train tracks. It works by partial matching too, so it's enough to write "tr" to match "train-tracks" for example.

@jeanluct
Copy link
Owner Author

jeanluct commented Dec 7, 2014

Should this be closed?

@jeanluct jeanluct added this to the release-2.2 milestone Dec 7, 2014
@jeanluct jeanluct added enhancement and removed new labels Dec 7, 2014
@mbudisic
Copy link
Collaborator

mbudisic commented Dec 8, 2014

entropy and complexity now have the new interface but this is definitely not implemented across the board. I doubt that we want to keep the full update of all the functions as "release-2.2" milestone, but perhaps it would be good to keep it up as a ticket.

@jeanluct jeanluct removed this from the release-2.2 milestone Dec 8, 2014
@jeanluct
Copy link
Owner Author

jeanluct commented Dec 8, 2014

Ok, deleted the milestone.

@jeanluct
Copy link
Owner Author

I guess this issue is still open? The constructors for braid and loop, for instance, are still mess. In fact they should probably be delegated to helper functions, to declutter things a bit. Anyways, this isn't very urgent.

@mbudisic
Copy link
Collaborator

OK, I did a bunch of work in iss80 branch on braid interface. There are several major novelties:

  • braid.braid uses databraid to process (XY,t) type of arguments
  • arguments braid(XY, projangle) are disallowed b/c it was too painful to constantly check whether second argument is a time vector or an angle. braid(XY, [], projangle) should be used instead
  • exceptions (errors) are slightly more sophisticated. It turns out that Matlab allows tacking "causes" to exceptions. E.g. if "badargument" exception is caused by exception "t vector is too short", then the two exceptions can be bundled together to give a clearer picture of what happened. Unit tests now allow for this
  • "named" braids, e.g., HK, halftwist, have been split into static function, and additional function that parses the name and calls the appropriate static has been added to declutter the constructor

All unit tests pass, except for taffy, for which databraid complains that the crossing times are not appropriately ordered. Could you please run the unit tests and see if this is a valid concern or not?

I didn't merge yet, because this is a pretty big change, and I wanted to see what's your opinion. (There is also a bit more work to be done, e.g., making sure documentation reflects the changes)

@jeanluct
Copy link
Owner Author

I'm not so happy about your change to the projection angle... it may be tedious but it is "internal". Isn't there a way to do this fairly cleanly?

Please don't merge into the develop branch until we test all this.

@mbudisic
Copy link
Collaborator

There is no way to do this cleanly - MATLAB's inputParser detects an argument based on its position and the argument's type (but not on type of some other argument). I still did it, since you seem to use the old interface often. It's kludgy but it's better than it was before (I think).

So, I reverted tests to the old interface - everything should work as it used to be. Errors are slightly different (the "addCause" allows you to describe what had happened, e.g., "badarg", and what caused that bad arg - e.g., an error deep in the sub-sub-invocation of processing that argument). This way we don't have to reinterpret the lower-level errors at every higher level to give them proper tags. (It results in a slightly more complicated unit test verifyError function, but that's it).

Still, I am not merging until we both agree with these changes.

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