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

Cleanup several things. #17

Merged
merged 27 commits into from
Jan 20, 2014
Merged

Cleanup several things. #17

merged 27 commits into from
Jan 20, 2014

Conversation

QuLogic
Copy link

@QuLogic QuLogic commented Jan 8, 2014

  • Fix some build rules so that things work in parallel.
  • Remove some old code.
  • Partly cleanup attenuation_compute_param.c as mentioned in remove copyrighted routines from attenuation_compute_param.c #13, though some of the NR code still needs replacing.
  • Use MPI_STATUS_IGNORE as in Fix call to MPI_RECV. #15.
  • Fix a few of the simpler warnings. (Not sure I want to go looking through all of them just yet.)
  • Remove Pyre from the build, as it's not used for anything.

In related terms, I'm wondering if I can:

  • Remove DATA/Database00000_left_edge_only, DATA/Database00000_right_edge_only? What are these even used for? I see they're read in the solver, but I'm not sure how they're produced.
  • Remove commented lines from UTILS/visualization/plot_wavefield.pl? @carltape?

Elliott Sales de Andrade added 25 commits January 6, 2014 23:31
We just use GitHub issues now.
Most of this comes from NR and should be replaced anyway.
The underscore situation should be detected by running ./configure.
These are already defined in math.h.
For system headers, they basically do nothing but take up extra lines.
Since the exectuables were never mentioned by full path, they would be
rebuilt unconditionally.
Without the correct dependencies, parallel builds fail due to missing
modules.
It's only used in the mesher, and the search path was wrong when used
here anyway.
Also, move them away from the NR functions.
Just placing the word exit there does nothing.
* MAX can be replaced by fmax.
* SIGN can be replaced by copysign.
* PYTHAG can be replaced by hypot.

The last two require C99, but since we require Fortran2003, I don't
think that's a big deal.
No more of this pseudo-Fortran indexing. It's odd and causes unnecessary
work (to be fixed).
ctype.h is required for isspace.
The status is not used, so this saves us an extra variable or two. As
suggested by @jedbrown in another PR.
Every time you configured with internal scotch, you'd get modifications
to its Makefiles which git would show as modified. This is troublesome
whenever you want to commit or change branches, because you'd have to
remember not to keep those changes.

The changes to the scotch Makefiles are independent of any options you
might pass to configure (except that they aren't done if you aren't
using internal scotch). We can just make them permanently in git,
especially because the changes have been made upstream in scotch as
well.
It's a bit confusing to call it 'makefile' when it's just a shell script
and not intended to be used with Make at all.
This should be more portable, but most scripts are really just a list of
commands, making little difference. Tested with bash and dash (which is
POSIX compliant, but doesn't implement most of bash's extensions).
I wonder if we should just remove this commented code, though.
@QuLogic
Copy link
Author

QuLogic commented Jan 12, 2014

Gosh darn it, this is opened to the wrong branch again! If you're merging, please change it to the QA branch first, since it doesn't appear I can change it myself.

Elliott Sales de Andrade added 2 commits January 11, 2014 21:01
Without this dependency, parallel builds fail due to missing scotchf.h.
The Pyrized version of the code was removed long ago, so these checks
don't do anything except change the default targets.
@komatits
Copy link
Contributor

We should try to make QA the default otherwise I am sure many users will
make the same mistake (without noticing it).
I am not sure how this can/could be done.

On 01/12/2014 02:57 AM, Elliott Sales de Andrade wrote:

Gosh darn it, this is opened to the wrong branch again! If you're
merging, please change it to the QA branch first, since it doesn't
appear I can change it myself.


Reply to this email directly or view it on GitHub
#17 (comment).

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr

@luet
Copy link
Contributor

luet commented Jan 13, 2014

It is possible to make QA the default for when people clone the repo. See
https://help.github.com/articles/setting-the-default-branch.
I see two ways of doing it:

  1. On the main repository:
    • pros: when a developer clones the main repo, his/her fork will be set
      so that QA is also the default branch for the clone.
    • cons: when a regular user clones the main repo they also end up in
      the QA branch automatically. That means that they won't be using the stable
      branch.
    • Each developer makes the change on their fork of the main repo.
    • pros:
    • cons: we can run into the same problems we have run into so far; that
      is a developer trying to commit to the master branch. But if it happens the
      maintainers will remind the developer to make the change. So this should
      converge in one iteration per developer :).

My preference goes to 2 (developers make the change on their forks). But I
can do 1 if you prefer.

Best,
David

On Sun, Jan 12, 2014 at 3:58 PM, Dimitri Komatitsch <
notifications@github.com> wrote:

We should try to make QA the default otherwise I am sure many users will
make the same mistake (without noticing it).
I am not sure how this can/could be done.

On 01/12/2014 02:57 AM, Elliott Sales de Andrade wrote:

Gosh darn it, this is opened to the wrong branch again! If you're
merging, please change it to the QA branch first, since it doesn't
appear I can change it myself.


Reply to this email directly or view it on GitHub
<#17 (comment)
.

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr


Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-32133781
.

David Luet
Linux Administrator/Software & Programming Analyst
Princeton University
Department of Geosciences & PICSciE
PICSciE (9 am - 1 pm): 609-258-8194
Geosciences (1 - 5 pm): 609-258-7945

@luet
Copy link
Contributor

luet commented Jan 13, 2014

What I said above is inconsistent with the workflow we established. The
developers should do their work in the devel branch and push them to the
main repository in the QA branch. The reason is that the devel branch
contains the latest developments that have been tested by buildbot. If
developers do their work from QA they might be working on untested code.

On Mon, Jan 13, 2014 at 10:53 AM, David Luet luet@princeton.edu wrote:

It is possible to make QA the default for when people clone the repo. See
https://help.github.com/articles/setting-the-default-branch.
I see two ways of doing it:

  1. On the main repository:
    • pros: when a developer clones the main repo, his/her fork will be
      set so that QA is also the default branch for the clone.
    • cons: when a regular user clones the main repo they also end up in
      the QA branch automatically. That means that they won't be using the stable
      branch.
    • Each developer makes the change on their fork of the main repo.
    • pros:
    • cons: we can run into the same problems we have run into so far;
      that is a developer trying to commit to the master branch. But if it
      happens the maintainers will remind the developer to make the change. So
      this should converge in one iteration per developer :).

My preference goes to 2 (developers make the change on their forks). But I
can do 1 if you prefer.

Best,
David

On Sun, Jan 12, 2014 at 3:58 PM, Dimitri Komatitsch <
notifications@github.com> wrote:

We should try to make QA the default otherwise I am sure many users will
make the same mistake (without noticing it).
I am not sure how this can/could be done.

On 01/12/2014 02:57 AM, Elliott Sales de Andrade wrote:

Gosh darn it, this is opened to the wrong branch again! If you're
merging, please change it to the QA branch first, since it doesn't
appear I can change it myself.


Reply to this email directly or view it on GitHub
<#17 (comment)
.

Dimitri Komatitsch
CNRS Research Director (DR CNRS), Laboratory of Mechanics and Acoustics,
UPR 7051, Marseille, France http://komatitsch.free.fr


Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-32133781
.

David Luet
Linux Administrator/Software & Programming Analyst
Princeton University
Department of Geosciences & PICSciE
PICSciE (9 am - 1 pm): 609-258-8194
Geosciences (1 - 5 pm): 609-258-7945

David Luet
Linux Administrator/Software & Programming Analyst
Princeton University
Department of Geosciences & PICSciE
PICSciE (9 am - 1 pm): 609-258-8194
Geosciences (1 - 5 pm): 609-258-7945

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

Successfully merging this pull request may close these issues.

3 participants