problem with recent projection changes
Ondrej Certik
I have several objections (and tips for improvements in the future) to
this recent patch:
https://github.com/hpfem/hermes/commit/5ab8624e6020959ea4c7f00429bbf16d9b6997a0
1) the log is insufficient. It says "H1D polishing.". Git logs should
be written as described here:
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
we don't need to follow all his ideas, but we should follow at least
the following:
* first line should be descriptive, without "." at the end, preferably
less than ~50 characters, so that it looks good in tools like gitk,
git log etc.
* empty line
* optional description of the commit
For simple commits, just one line is sufficient, for complex commits
like the above, at least a paragraph is needed, so that people like me
when things go wrong can figure out what is going on
2) the commit is too big, mixing lots of changes into one. It should
have been split into several smaller commits, e.g. one changing the
projections, another one changing all the examples (which is unrelated
to the projections, as you do lots of changes like
-double Val_dir_left = 0;
-double Val_dir_right = 0;
+Tuple<BCSpec *>DIR_BC_LEFT = Tuple<BCSpec *>(new BCSpec(0,0));
+Tuple<BCSpec *>DIR_BC_RIGHT = Tuple<BCSpec *>(new BCSpec(0,0));
), another one fixing Space.init and so on.
3) assemble_projection_matrix_rhs() was removed:
https://github.com/hpfem/hermes/commit/5ab8624e6020959ea4c7f00429bbf16d9b6997a0#diff-22
I am -1 on that. This function was used in the Python wrappers and was
well tested in the python tests. What was the reason for its removal?
I assume the same functionality can now be achieve using the new API.
Can you help me do that? Alternatively, I can put back my old code, as
my schroedinger module depends on this.
4) I wrote a very nice comment with C++ examples how to use this ExactFunction:
typedef void(*ExactFunction)(int n, double x[], double f[], double dfdx[]);
this comment was removed (see the above link). I am -1 on that. I
propose to put it back in, unless there is some good reason why no
comment is better than what was there already.
--------------------
The motivation + explanation + comments to 3) and 4) need to go into
the git commit log. E.g. ""H1D polishing." is not enough. There should
be a patch for 4), with a commit log:
"""
Removing documentation for the ExactFunction() function definition
This comment didn't make any sense, and was so bad, that no comment is
better than this. So I am removing it for now, until someone can fix
this and write something better.
"""
I am joking a bit here, but you get the idea. Another patch for 3)
should have had a commit log:
"""
Reworking API for projections
The function assemble_projection_matrix_rhs() was removed and instead
the same functionality can be achieved using the general
OGProjection() classes. See the documentation for the OGProjection()
class in the source code for examples of usage.
"""
And a documentation for OGProjection should have been added.
You don't need to follow exactly my instructions, but the quality of
the commits should improve. It will cost you 30s to write a better log
message, and another minute or two to split the patch. Plus it forces
you to think about the commit changes and document them, both in the
code and in the log.
Now I spent over an hour, trying to understand what is going on, and
it is still not clear to me (the motivation, e.g. whether it is
something that you omitted by a mistake, or on purpose, and what
purpose it is, so that I can see what the best way for me to proceed
is), so I need to ask here.
I just need a little help with the assemble_projection_matrix_rhs()
and I should be able to enable all the Python wrappers again.
Ondrej
Lukas Korous
Hi,
I have several objections (and tips for improvements in the future) to
this recent patch:
https://github.com/hpfem/hermes/commit/5ab8624e6020959ea4c7f00429bbf16d9b6997a0
1) the log is insufficient. It says "H1D polishing.". Git logs should
be written as described here:
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
we don't need to follow all his ideas, but we should follow at least
the following:
* first line should be descriptive, without "." at the end, preferably
less than ~50 characters, so that it looks good in tools like gitk,
git log etc.
* empty line
* optional description of the commit
For simple commits, just one line is sufficient, for complex commits
like the above, at least a paragraph is needed, so that people like me
when things go wrong can figure out what is going on
First, thanks for your ideas.
In retrospect (I would not call the patch recent) you are right that "H1D polishing" does not really say much, it was during the API was being reworked, so there were a lot of changes that were discussed with Pavel.
2) the commit is too big, mixing lots of changes into one. It should
have been split into several smaller commits, e.g. one changing the
projections, another one changing all the examples (which is unrelated
to the projections, as you do lots of changes like
-double Val_dir_left = 0;
-double Val_dir_right = 0;
+Tuple<BCSpec *>DIR_BC_LEFT = Tuple<BCSpec *>(new BCSpec(0,0));
+Tuple<BCSpec *>DIR_BC_RIGHT = Tuple<BCSpec *>(new BCSpec(0,0));
), another one fixing Space.init and so on.
Yes, that's a good idea, smaller commits. I will keep that in mind.
3) assemble_projection_matrix_rhs() was removed:
https://github.com/hpfem/hermes/commit/5ab8624e6020959ea4c7f00429bbf16d9b6997a0#diff-22
I am -1 on that. This function was used in the Python wrappers and was
well tested in the python tests. What was the reason for its removal?
I assume the same functionality can now be achieve using the new API.
Can you help me do that? Alternatively, I can put back my old code, as
my schroedinger module depends on this.
The class now serves not only projecting an exact function as before that commit. As for the functionality : there is an overloaded method project_global that can be used in projecting exact functions. However, if you need it for some purpose, I would go with putting back your old code as the current functionality hides the projection matrix from the user.
4) I wrote a very nice comment with C++ examples how to use this ExactFunction:
typedef void(*ExactFunction)(int n, double x[], double f[], double dfdx[]);
this comment was removed (see the above link). I am -1 on that. I
propose to put it back in, unless there is some good reason why no
comment is better than what was there already.
Sorry about that, this was surely deleted by mistake, I would not delete such a nice comment.
If you need anything else, drop me a line please.
Lukas
Ondrej
Ondrej Certik
On Sun, Dec 5, 2010 at 1:55 AM, Lukas Korous <lukas....@gmail.com> wrote:
> Hi,
>
> On Sun, Dec 5, 2010 at 7:57 AM, Ondrej Certik <ond...@certik.cz> wrote:
>>
>> Hi,
>>
>> I have several objections (and tips for improvements in the future) to
>> this recent patch:
>>
>>
>> https://github.com/hpfem/hermes/commit/5ab8624e6020959ea4c7f00429bbf16d9b6997a0
>>
>> 1) the log is insufficient. It says "H1D polishing.". Git logs should
>> be written as described here:
>>
>> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>>
>> we don't need to follow all his ideas, but we should follow at least
>> the following:
>>
>> * first line should be descriptive, without "." at the end, preferably
>> less than ~50 characters, so that it looks good in tools like gitk,
>> git log etc.
>> * empty line
>> * optional description of the commit
>>
>> For simple commits, just one line is sufficient, for complex commits
>> like the above, at least a paragraph is needed, so that people like me
>> when things go wrong can figure out what is going on
>
> First, thanks for your ideas.
> In retrospect (I would not call the patch recent) you are right that "H1D
> polishing" does not really say much, it was during the API was being
> reworked, so there were a lot of changes that were discussed with Pavel.
No worries, thanks for the changes that you made.
>
>>
>> 2) the commit is too big, mixing lots of changes into one. It should
>> have been split into several smaller commits, e.g. one changing the
>> projections, another one changing all the examples (which is unrelated
>> to the projections, as you do lots of changes like
>> -double Val_dir_left = 0;
>> -double Val_dir_right = 0;
>> +Tuple<BCSpec *>DIR_BC_LEFT = Tuple<BCSpec *>(new BCSpec(0,0));
>> +Tuple<BCSpec *>DIR_BC_RIGHT = Tuple<BCSpec *>(new BCSpec(0,0));
>> ), another one fixing Space.init and so on.
>
> Yes, that's a good idea, smaller commits. I will keep that in mind.
>
>>
>> 3) assemble_projection_matrix_rhs() was removed:
>>
>>
>> https://github.com/hpfem/hermes/commit/5ab8624e6020959ea4c7f00429bbf16d9b6997a0#diff-22
>>
>> I am -1 on that. This function was used in the Python wrappers and was
>> well tested in the python tests. What was the reason for its removal?
>> I assume the same functionality can now be achieve using the new API.
>> Can you help me do that? Alternatively, I can put back my old code, as
>> my schroedinger module depends on this.
>
> The class now serves not only projecting an exact function as before that
> commit. As for the functionality : there is an overloaded method
> project_global that can be used in projecting exact functions. However, if
> you need it for some purpose, I would go with putting back your old code as
> the current functionality hides the projection matrix from the user.
I need the projection matrices, as I solve them myself using numpy,
and I wrote bunch of tests for that. It helps to have the code
modular, so that it allows that. I am sure it can be achieved with the
OGProjection class too somehow, but there seems some hardwired things
for up to 4 (why four?) solutions and other things, so I just put my
old code back in and things work perfectly and all Python tests pass
again. That was my achievement yesterday at 5am.
>
>>
>> 4) I wrote a very nice comment with C++ examples how to use this
>> ExactFunction:
>>
>> typedef void(*ExactFunction)(int n, double x[], double f[], double
>> dfdx[]);
>>
>> this comment was removed (see the above link). I am -1 on that. I
>> propose to put it back in, unless there is some good reason why no
>> comment is better than what was there already.
>
> Sorry about that, this was surely deleted by mistake, I would not delete
> such a nice comment.
I put it back in.
I will see. Now I need to figure out how to use the new changes in
WeakForm and DiscreteProblem, but it should be easy hopefully.
Ondrej