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

Convert FAR to units of Hz when dumping to XML #218

Merged
merged 1 commit into from
Aug 27, 2015

Conversation

spxiwh
Copy link
Contributor

@spxiwh spxiwh commented Aug 26, 2015

This is a small patch to convert the units of combined_far in our XML coinc output files to Hz. Note that our conversion between seconds and years is slightly unclear as described in #217, this should be fixed.

@tdent
Copy link
Contributor

tdent commented Aug 26, 2015

Please use LAL SI Julian year Ian .. and check the order of magnitude is correct by running the patched code on the ER7 events. Are you sure you should be multiplying by the LAL conversion factor rather than dividing?

@spxiwh
Copy link
Contributor Author

spxiwh commented Aug 26, 2015

Hi Tom,

I have updated the patch to divide by the rescaling factor, which now gives sane values (ie. most significant event is 9.78E-7). I agree that we should use LAL SI Julian year, but if I use a different value from what Alex has when computing the FAR then this will not be valid.

Cheers
Ian

@tdent
Copy link
Contributor

tdent commented Aug 26, 2015

.. Then make the change in both codes in the same request, even preferably in the same commit, so that there won't be a state of the code where there is an inconsistency. Then we can close #217.

The FAR calculation works in seconds anyway, it's only converted for output / storage / display purposes.

@ahnitz
Copy link
Member

ahnitz commented Aug 26, 2015

Thanks for fixing this. If we can do the change all at once to use the correct number that would be best.

@spxiwh spxiwh assigned ahnitz and unassigned tdent Aug 27, 2015
@spxiwh
Copy link
Contributor Author

spxiwh commented Aug 27, 2015

Okay I've converted the patch to use lal.YRJUL_SI as requested and also changed this in the HDF FAR codes.

I'm assigning this now to Alex to check that there isn't anywhere else in the HDF code where this number is hardcoded? Tom, please speak up if you are not happy with this now.

@tdent
Copy link
Contributor

tdent commented Aug 27, 2015

Thanks Ian. I have checked the lal.YRJUL_SI constant is a python float, so there won't be integer division issues (which might have occurred since the Julian year is an exact number of seconds).
Could you please roll all 3 commits together with git rebase --interactive and update the branch via push -f, that would make the history a lot cleaner/more comprehensible and avoid buggy intermediate states.

@spxiwh
Copy link
Contributor Author

spxiwh commented Aug 27, 2015

This is now rebased into a single commit

@ahnitz
Copy link
Member

ahnitz commented Aug 27, 2015

The commits fix the FAR assignment codes in the right place, so I'm happy for this to merge.

ahnitz added a commit that referenced this pull request Aug 27, 2015
Convert FAR to units of Hz when dumping to XML
@ahnitz ahnitz merged commit 4ab759a into gwastro:master Aug 27, 2015
@spxiwh spxiwh deleted the far_conversion_fix branch September 8, 2015 09:11
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