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

#148 Improve functionality of the config parsing #241

Merged
merged 274 commits into from
Sep 10, 2012
Merged

#148 Improve functionality of the config parsing #241

merged 274 commits into from
Sep 10, 2012

Conversation

rmjarvis
Copy link
Member

@rmjarvis rmjarvis commented Sep 5, 2012

There are now configuration files (both yaml and json) that build the same files as those built by 8 current demo scripts. These are in examples/*.yaml and examples/json/*.json. (I think it's fair to say that yaml is our preferred format, hence the json files being relegated to a subdirectory.)

There are two executable scripts in the examples directory called check_yaml and check_json, which run both the Demo.py files and also the versions that parse the .yaml or .json config files (respectively) and compare their outputs. The scripts aren't very sophisticated, but if the last line of output is

Checking diffs: (No output means success)

then everything is working correctly.

There is also fairly complete documentation on the wiki, which could be considered part of the pull request, even though it won't show up in the diff.

The branch to be merged is #148, but other issues that could likely be closed after this is merged are:

…e objects by my own definition, also added un-setness testing for size params in _parse_sizes

(#195)
@barnabytprowe
Copy link
Member

That's fine, lam_over_diam is good. Sorry for missing that query (I'd
noticed the question but didn't succeed in getting back to it).

On Fri, Sep 7, 2012 at 6:18 AM, Mike Jarvis notifications@github.comwrote:

lam_over_D (for Airy and OpticalPSF) is the only parameter that isn't pure
lowercase. It seems like we might want to change that. Suggestions:
lam_over_diam, size, rayleigh_size, resolution, tel_resolution...

Since no one weighed in on this point, I'm going to go with lam_over_diam,
unless there are any objections.


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

Barnaby Rowe
Postdoctoral Research Associate

Department of Physics & Astronomy
University College London
Gower Street
London WC1E 6BT
United Kingdom

@rmandelb
Copy link
Member

rmandelb commented Sep 7, 2012

Yes, sorry I missed that query. I'm fine with whatever you want to do for lam_over_D.

At this point, I think that this is quite a useful expansion of the config system, and a really amazing set of examples, thank you Mike! I would be fine with merging whenever. I suggest the following schedule -

And then I think we can focus on the remaining issues for releasing the code within GREAT3, namely the documentation drive (we have 5 volunteers to go over the galsim/ python docstrings) and installation procedures.

It's coming together...
(sorry for getting slightly off-topic on the pull request!)

@rmjarvis
Copy link
Member Author

rmjarvis commented Sep 7, 2012

I think I addressed all comments and suggested changes. So I'm fine with merging whenever, but also happy to wait for more people to have a chance to comment.

@barnabytprowe
Copy link
Member

OK, so I think we're still all good to merge here? I'll leave it until mid-morning EDT to see if there are any objections, then will merge... But it's OK to wait longer if people want more time, so please just let us know.

Thanks for all the work on this branch everybody, particularly Mike.

@rmandelb
Copy link
Member

Yes, I think we're fine to merge.

@rmandelb
Copy link
Member

(and once we do, I will make a pull request for 214, which i branched off of this branch)

@rmandelb
Copy link
Member

Barney, if you're merging, will you also take care of the list (in the 1st message on the pull request) of issues that are to be closed / modified because of this one? (153, etc.)

@barnabytprowe
Copy link
Member

Will do!

Barnaby Rowe
Postdoctoral Research Associate

Department of Physics & Astronomy
University College London
Gower Street
London WC1E 6BT
United Kingdom

On 10 Sep 2012, at 14:59, rmandelb wrote:

Barney, if you're merging, will you also take care of the list (in the 1st message on the pull request) of issues that are to be closed / modified because of this one? (153, etc.)


Reply to this email directly or view it on GitHub.

@barnabytprowe
Copy link
Member

OK, I think I've closed all the related issues... Did one last compilation and scons tests run, all ok. Here goes with the merge!

barnabytprowe added a commit that referenced this pull request Sep 10, 2012
#148 Improve functionality of the config parsing
@barnabytprowe barnabytprowe merged commit 0393b46 into master Sep 10, 2012
@rmandelb rmandelb deleted the #148 branch July 4, 2014 14:55
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.

4 participants