Add a New Comment (Rich Markup)
Comment Type & Canned Response: None None > Multiple Canned Responses Fixed in development Crash with no stack trace Already fixed in newer version Fixed in stable Bad description Bad description and crash Bad stack trace Obsolete version Duplicate and not fixed Duplicate and needs more info Duplicate and fixed Need info and old
( Jump to the original submission )
the two versions of your patch build on each other. someone cannot apply the last version of your patch without having applied the first one. Note also since the two patches you uploaded are named the same thing, it may not be obvious that one depends on the other.
It's best when updating a patch to roll back to a clean state, then apply all of the changes so they are contained in a single patch file. (sometimes this also needs to be done to 'refresh' patches in old bug reports that no longer apply to the current source anymore).
You do not need to reclone the source. It may not be the best way (others more experienced can correct/add to my suggestions) but you can 'rebase' a patch using 'hg strip' and 'hg patch' commands:
(This assumes you aren't using branches to make changes alonge another developmnet line, and also the process will destroy any pending changes you've made, so don't do it if you have other changes not captured elsewhere, exported to a patch, etc.)
To get back to the current development tip (where you would be from a fresh source clone) 'hg strip -r tip' will step you back one commit at a time. (Note it will also create backup bundles for recovery unless you use --no-backup, and that it will stop you if you have any uncommitted edits which you can override (carefully!) with a -f.)
depending on how many draft commits you have on your clone, at some point a summary will show commit: (clean) and update: (current), e.g,:
$ hg summ parent: 30992:4ef25c610433 tip format: Add 'default' format option (bug #62430) branch: default bookmarks: @ commit: (clean) update: (current)
if you know how far back you have to go, you can specify the revision to go back to in the strip command instead of '-r tip'.
from there, 'hg pull' and 'hg update' to be sure you have the latest revision.
then, 'hg patch --no-commit <filename>' for the patches you want to apply (in this case, reapply your first then your second patch). make any further changes you want, then do a single 'hg commit' to capture all of the changes, and 'hg export' a single patch others can use to test your changes.
generally, after exporting and uploading a patch, unless I'm testing further related changes I will repeat the 'hg strip' of my changes so that the next thing I'm doing will be based on a clean source. I can always reapply my own patch as above to make updates or changes. Branches can be a better way to manage multiple independent tasks as long as you're still generating 'clean' patches for others to test.
comment #9:
> Your patch dous not apply to current sources, > it looks like it applies to a file that was already modified by yourself. > > Could you please provide a changeset with respect to the current development branch?
The source code I had worked on was obtained following the instructions here : https://wiki.octave.org/Building, i.e cloned from https://www.octave.org/hg/octave . When you say it doesn't apply to current sources, do you mean I should just pull the latest changes and commit my patch again or do I need to clone the development branch?
Your patch dous not apply to current sources, it looks like it applies to a file that was already modified by yourself.
Could you please provide a changeset with respect to the current development branch?
> > which although seem to have solved for empty matrices, shows slight differences when dimensions are specified.
I have understood the mistake in my understanding, and by changing the code after the reduction has taken place for the special case of an empty matrix, I was able to procure these results :
octave:1> dot([],[]) ans = [](0x0) octave:2> dot([],[],1) ans = [](1x0) octave:3> dot([],[],2) ans = [](0x1)
which are completely compatible with what Matlab does. I have added some tests to check for empty inputs, and my changeset passes all pre-existing and new tests. Please have a look!
(file #53105)
As I worked on bug #61690, to make functions sum, prod etc that use function do_mx_red_op to perform reductions, and was able to succesfully make it consistent with Matlab, I tried to work and see if a similar logic would work to solve this as well.
Going through the comments,
comment #2:
> Yes, that's what I found as well. This means that if we would choose to go with Matlabs version it is just a matter of adding a check for "dim < 0" in the following if-statement. > > For "dot" we add, right after the definition of dimx and dimy
this unfortunately is not possible or understandable to me as this code doesn't seem to exist in dot.cc file anymore. Instead, I worked on the function get_red_dims , and was able to procure these results :
octave:1> dot([],[]) ans = [](0x0) octave:2> dot([],[],1) ans = [](0x0) octave:3> dot([],[],2) ans = [](0x1)
which although seem to have solved for empty matrices, shows slight differences when dimensions are specified. This is because my code change looks like :
if(x(1)==0 && dim == 0){ printf("here"); z(0) = 0;} else{ for (int i = 0; i < nd; i++) { if (i < dim) { z(i) = x(i); tmp_m *= x(i); }
and when dim is specified as 1 by the user, it is converted to 0 for index conventions in vectors.
The only reason I am doing this is to make it compatible with Matlab, which seems to do this :
>> dot([],[]) ans = 0 >> dot([],[],1) ans = 1×0 empty double row vector >> dot([],[],2) ans = 0×1 empty double column vector
How exactly would we want to proceed? And if we want to make this reciprocate Matlab behaviour, does anyone have an idea of how we could differenciate between dot([],[]) and dot([],[],1), as dim=1 doesn't seem to work.
As mentioned this needs to be implemented in the dot.cc file and cannot be done as a m-file, mainly due to performance reasons.
Apart from that I can also give you some feedback on your code. From what I can tell it has the required functionality, but there are things that can be improved in the code.
- Your error messages give error messages when called. The problem is you calling function message(), which does not exist. One example on how to do it can be found in this file [1]
- Take a look at the Octave style guide [2]. For example it is recommended to use double quoted strings and end blocks with a specific ending, for example endif for if-blocks.
If you want to try and modify the dot.cc file you have to be able to build Octave yourself. I don't know which operating system you are using, but some general info can be found here [3]. It can however be a bit complicated, especially if something goes wrong.
[1]: https://sourceforge.net/p/octave/interval/ci/default/tree/inst/@infsup/private/str2decimal.m [2]: https://wiki.octave.org/Octave_style_guide [3]: https://wiki.octave.org/Build_from_source
> Quote SUDHIR KUMAR SUMAN > > Can't we just remove that . cc file and replace with it i.e . m file.
Please reply only via this bug-tracker;
https://savannah.gnu.org/bugs/?51333
The email is just for notification, not for answering.
"dot" is a core function, that means it should be super fast, as it is often called. M-files are slower in execution (they require interpretation). Furthermore, this function should be offered to users that compile against libinterpreter (see standalone programs [1]). Thus your solution is unfortunately no feasible solution.
Maybe you can try to solve this issue on C++ level, not just as a quick fix, but this requires some more time.
[1]: https://octave.org/doc/interpreter/Standalone-Programs.html#Standalone-Programs
Thank you for your interest and code submission. I am sure your code works pretty well, but this bug requires a solution on C++ level in dot.cc.
https://hg.savannah.gnu.org/hgweb/octave/file/a73dedb0d9b5/libinterp/corefcn/dot.cc
I have gone through the bug #51333 and have implemented code for dot function. Similarly it can be implemented for others.I have also consider case when dimension is negative. The link for the code is : https://github.com/Alok009/Octave/blob/master/dot.m
Output from octave:
>> dot([],[])
ans = [](1x0)
Output after implimenting
>> dott([],[])
ans = 0
Yes, that's what I found as well. This means that if we would choose to go with Matlabs version it is just a matter of adding a check for "dim < 0" in the following if-statement.
For "dot" we add, right after the definition of dimx and dimy,
// Inconsistency: dot ([], []) = 0 if (dimx.ndims () == 2 && dimx(0) == 0 && dimx(1) == 0 && dimy.ndims () == 2 && dimy(0) == 0 && dimy(1) == 0) { dimx(1) = 1; dimy(1) = 1; }
if we go with the version Octave has now. If we choose the matlab version we also add "nargin == 2" to the condition.
I don't know if we should go with the Matlab version, but either way I think it makes sense to do the changes to "dot".
Applies for the development version as well.
Digging into the code of "sum", "xsum", "prod", "sumsq" of an double NDArray, I found the inline function "do_mx_red_op" to be called in any case. According to a comment
https://hg.savannah.gnu.org/hgweb/octave/file/tip/liboctave/operators/mx-inlines.cc#l1537
// M*b inconsistency: sum ([]) = 0 etc.
it has to do with the described problem.
For "dot" the situation might be easier, as it is a seperate file which could check for empty input values:
https://hg.savannah.gnu.org/hgweb/octave/file/tip/libinterp/corefcn/dot.cc#l67
Some of the reduction functions have inconsistent handling of empty matrices. They also differ slightly to how Matlab handles it. The function I have looked at are sum, sumsq, prod and dot but it might hold true for other reduction functions as well.
Both sum and prod have the convention that the empty matrix returns the unit, i.e.
> sum ([]) ans = 0 > sumsq ([]) ans = 0 > prod ([]) ans = 1
But this does not hold true for dot
> dot ([], []) ans = [](1x0)
I think it would be more natural for dot to also return 0 here. This is also how Matlab does it
## In Matlab > dot ([], []) ans = 0
Further I noticed that Matlab and Octave handles the empty matrix differently when a dimension is specified. In Octave we have
> sum ([]) ans = 0 > sum ([], 1) ans = 0 > sum ([], 2) ans = [](0x1)
Looking at the code for Octaves reduction function we in fact have a explicit conversion from [] to a matrix of size 1x0 (commented as an inconsistency). In Matlab we instead have
## In Matlab > sum ([]) ans = 0 > sum ([], 1) ans = [](1x0) > sum ([], 2) ans = [](0x1)
When no dimension is specified it handles the empty matrix in a special way. But when a dimension is specified it handles it like any other matrix.
(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)
Attach Files: Comment:
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
Only project members can vote.
Please enter the title of George Orwell's famous dystopian book (it's a date):
Follow 4 latest changes.
Copyright © 2023 Free Software Foundation, Inc. Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved. The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team. Source Code
Powered by Savane 3.9