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

AbstractRealInterval constructors #28

Closed
bdezonia opened this issue Aug 29, 2013 · 3 comments
Closed

AbstractRealInterval constructors #28

bdezonia opened this issue Aug 29, 2013 · 3 comments

Comments

@bdezonia
Copy link
Contributor

I have been using RealIntervals for some test code. I was looking at AbstractRealInterval and have a question about two of the constructors.

The AbstractRealInterval(RealInterval other) constructor seems to incorrectly initialize min and max to 0's rather than the min and max of the other interval.

The AbstractRealInterval(double[] dimensions) constructor subtracts one from each dimension when setting min and max. This makes sense for an long[] dimension input but not for a double[] input. It has the paradoxical result that creating a real dim of width 5 reports realMin as 0 and realMax as 4 and an effective width of 4.

Can someone (Steffi you wrote this I think) review these and comment? Thanks.

@tpietzsch
Copy link
Member

Thanks for pointing these out, Barry! Both are clearly bugs. I just pushed the fix.

I fixed the copy constructor and removed the AbstractRealInterval(double[] dimensions) constructor.
The way we defined it, min and max are inclusive. So for (integer) Interval with min=0, dimensions=max+1.
Because every Interval is also a RealInterval, we decided that RealInterval has no dimensions() method (exactly because the dimensions=max+1 makes no sense here). Therefore the (double[] dimensions) constructor also makes no sense.

The alternative would have been to define min as inclusive and max as exclusive. Maybe we should have done that, now it would be a lot of work to change it (but not impossible)

@bdezonia
Copy link
Contributor Author

bdezonia commented Sep 3, 2013

Thanks for the fixes Tobias. Shall we close this issue? Or did you want feedback on the exclusive max idea?

@tpietzsch
Copy link
Member

Sorry, Barry. I just forgot. I'll close the issue now. Feedback on exclusive vs inclusive max is welcome though :)

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

No branches or pull requests

2 participants