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

Added a script to dump the hazard outputs needed for the risk #1096

Merged
merged 25 commits into from May 13, 2013

Conversation

micheles
Copy link
Contributor



def zipdir(dirpath):
"zip the contents of a directory in a zipfile and remove the directory"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring style is inconsistent within this file, as well as the rest of the codebase.

@larsbutler
Copy link
Contributor

Why is this tool needed?
Is there a related bug in Launchpad?

@micheles
Copy link
Contributor Author

This was a request by Paul. Please consider the code here as a preview, we have yet to test it in real life use cases. I just opened a pull request since it was the easiest way to show it to Paul. The idea is to perform an expensive hazard calculation on Hope, zip the outputs and give them to a scientist which then can run a fast risk calculation on his laptop. I will open a bug later on.

# -*- coding: utf-8 -*-
# vim: tabstop=4 shiftwidth=4 softtabstop=4

# Copyright (c) 2010-2013, GEM foundation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, the copyright date should just be 2013.
foundation should be capitalized.

@larsbutler
Copy link
Contributor

Can you please file the bug before this is merged? Any non-trivial pull request should have a Launchpad bug to document the change (including requirements, etc.).

Without a description of the use cases, there is no way for a reviewer to know if the code is good or not.

@micheles
Copy link
Contributor Author

Fair enough.

@micheles
Copy link
Contributor Author

Small fixes and added the corresponding launchpad issue

with the same id. If you think that the hazard calculation on your database
is not important and can removed together with all of its outputs, then
remove it by using ``bin/openquake --delete-hazard-calculation`` (which
must be run by an user with sufficient permissions). Then run again
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/by an user/by a user/

@larsbutler
Copy link
Contributor

@micheles The bug you linked doesn't really give any more details than your comments thus far. In fact it looks just like a copy/paste excerpt from one of your comments above.

The information I'm interested in is about use cases, for example. Like, what types of calculations do users expect to dump/restore? All calculation modes, or just some of them? Do we expect to export statistical results as well, such mean and quantile hazard curves? That's what I would expect to see in the bug report.

Also, I've marked the bug as Confirmed and have assigned it to you. Please assign a priority to the bug.

follow this procedure.

1. go in the directory where this README is
2. run ``source restore.sh``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is restore.sh?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see that it is generated below. Can you please add a comment explaining that this file is generated, and when/how?

@larsbutler
Copy link
Contributor

Please rebase this branch on master; there are some spurious test failures.

@matley
Copy link
Contributor

matley commented Apr 12, 2013

How this change relates to https://bugs.launchpad.net/oq-engine/+bug/1167693 ? It seems that the produced output is not compatible with the PGImporter.

Please, also update the launchpad issue by linking this pull request.

@micheles
Copy link
Contributor Author

This is not directly related to https://bugs.launchpad.net/oq-engine/+bug/1167693. The approached used in that ticket is only for tests, it is not a valid mechanism for importing from a db to another one. This pull request solves the much easier problem of importing data into an empty database, when there are no id conflicts with the auto incremental fields. This is clearly not enough, but since the general problem is difficult it will be faced later on in another ticket.

@larsbutler
Copy link
Contributor

I'm trying to use the script, and I'm getting this error:

$ python openquake/engine/tools/dump_hazards.py 111
Traceback (most recent call last):
  File "openquake/engine/tools/dump_hazards.py", line 279, in 
    arg.user, arg.password)
  File "openquake/engine/tools/dump_hazards.py", line 227, in main
    dbname=dbname, user=user, password=password)
TypeError: argument 3 must be string, not None

shutil.rmtree(out_dir)
os.mkdir(out_dir)

conn = psycopg2.connect(host=None if host == 'localhost' else host,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the host set to None is host "localhost"? Why can't we just let it be "localhost"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why aren't we leveraging the Django-managed DB connections for this? All other command line interface options (export, delete, etc.) do not require a hostname or credentials.

@micheles
Copy link
Contributor Author

The dump/restore procedures have been completely rewritten; I also rewrote the bug description.
I could even close this pull request and open a new one if you feel this is more clear.

@matley
Copy link
Contributor

matley commented May 13, 2013

LGTM

micheles added a commit that referenced this pull request May 13, 2013
Added a script to dump the hazard outputs needed for the risk
@micheles micheles merged commit 22ae5b2 into master May 13, 2013
@micheles micheles deleted the dump_hazards branch May 13, 2013 13:31
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.

None yet

3 participants