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

Bremstrahlung spectrum, and fixes to make integration of domains easier #183

Merged
merged 37 commits into from
Nov 2, 2015

Conversation

Higginbottom
Copy link
Collaborator

No description provided.

@jhmatthews
Copy link
Collaborator

This looks good. Only question was what we should be doing temporarily for dielectronic recombination? At the moment it looks like you've made it so it is set to zero by default?

@jhmatthews
Copy link
Collaborator

Quick note for future reference that the lines added in matrix_ion, i.e.

   /* free memory */
  gsl_vector_free (test_vector);
  gsl_matrix_free (test_matrix);
   gsl_vector_free (populations);          

stop a fairly major memory leak, because just using free(test_vector) doesn't work

@Higginbottom
Copy link
Collaborator Author

Yes, at the moment I'm setting it to zero. I have no understanding at
the present time how to do it right - so I think its best to turn it
off. We don't make any photons to balance it anyway.....

On 29/10/2015 15:24, James Matthews wrote:

This looks good. Only question was what we should be doing temporarily
for dielectronic recombination? At the moment it looks like you've
made it so it is set to zero by default?


Reply to this email directly or view it on GitHub
https://github.com/agnwinds/python/pull/183#issuecomment-152213956.

@Higginbottom
Copy link
Collaborator Author

I've also added a new error condition in matrix.c, and a communicated
error code so I know if the matrix ion bad returns are due to something
nasty or 'just' because the absolute comparison fails (normally the case)

On 29/10/2015 15:27, James Matthews wrote:

Quick note for future reference that the lines added in matrix_ion, i.e.

/* free memory */

gsl_vector_free (test_vector);
gsl_matrix_free (test_matrix);
gsl_vector_free (populations);

stop a fairly major memory leak, because just using free(test_vector)
doesn't work


Reply to this email directly or view it on GitHub
https://github.com/agnwinds/python/pull/183#issuecomment-152214772.

@kslong
Copy link
Collaborator

kslong commented Oct 29, 2015

We need to spend the time to do this right. This is the sort of thing I objected to last week. Short term goals trumping long term needs.

Sent from my iPad

On Oct 29, 2015, at 10:34 AM, Higginbottom <[email protected]mailto:[email protected]> wrote:

Yes, at the moment I'm setting it to zero. I have no understanding at
the present time how to do it right - so I think its best to turn it
off. We don't make any photons to balance it anyway.....

On 29/10/2015 15:24, James Matthews wrote:

This looks good. Only question was what we should be doing temporarily
for dielectronic recombination? At the moment it looks like you've
made it so it is set to zero by default?


Reply to this email directly or view it on GitHub
https://github.com/agnwinds/python/pull/183#issuecomment-152213956.


Reply to this email directly or view it on GitHubhttps://github.com/agnwinds/python/pull/183#issuecomment-152217189.

@Higginbottom
Copy link
Collaborator Author

I agree. This is a quick message about this merge. Once I get the hydro stuff running, I'll have several free days to investigate how to do dielectronic recomb right. I'm moderately sure that the insight I'll get into my hydro work won't be changed massively by changes to dr cooling. So the two can run concurrently.
N

Sent from my iPhone

On 29 Oct 2015, at 22:48, kslong [email protected] wrote:

We need to spend the time to do this right. This is the sort of thing I objected to last week. Short term goals trumping long term needs.

Sent from my iPad

On Oct 29, 2015, at 10:34 AM, Higginbottom <[email protected]mailto:[email protected]> wrote:

Yes, at the moment I'm setting it to zero. I have no understanding at
the present time how to do it right - so I think its best to turn it
off. We don't make any photons to balance it anyway.....

On 29/10/2015 15:24, James Matthews wrote:

This looks good. Only question was what we should be doing temporarily
for dielectronic recombination? At the moment it looks like you've
made it so it is set to zero by default?


Reply to this email directly or view it on GitHub
https://github.com/agnwinds/python/pull/183#issuecomment-152213956.


Reply to this email directly or view it on GitHubhttps://github.com/agnwinds/python/pull/183#issuecomment-152217189.

Reply to this email directly or view it on GitHub.

Higginbottom added a commit that referenced this pull request Nov 2, 2015
Bremstrahlung spectrum, and fixes to make integration of domains easier
@Higginbottom Higginbottom merged commit d7fecba into sirocco-rt:dev Nov 2, 2015
@jhmatthews
Copy link
Collaborator

For future reference, I don't really understand why debugging code has been committed to this branch, and also domain2. If you need to debug in git for say getting code between remote devices then I'd suggest using temporary branches for that

# starting in dev/domain2/development branch, having just coded something new
git commit -am 'made a new change'
# I want to debug it, so if I need versioning I can do this in a separate branch
git checkout -b debug_new_change
# debug, commit, push and pull at whim on this branch
# when you're ready to make the actual change simply switch back to dev or whatever
git checkout dev
git commit -am "Debugged and fixed"

this avoids need to have a load of debugging statements in the main commit history which makes it nigh impossible to know what has actually gone on.

Likewise, this change and the domain2 change seem to involve some 'overwriting' of code manually, which we should try to avoid. Otherwise we lose commit histories and some of the benefit of git is negated.

Rant over, sorry.

@jhmatthews
Copy link
Collaborator

@Higginbottom -- this pull request seems to have created a problem with domain2. Not sure why yet. May need to revert this pull request.

In other news, 'git rebase' is an option to avoid messy commit histories - you can choose to squash commit histories into a few concise well described commits with git rebase -i -- see here:

https://github.com/ginatrapani/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

@Higginbottom
Copy link
Collaborator Author

This makes no sense to me. Are you saying I need to make a branch of my fork to debug in. Then merge to my fork before merging to the main repo? Ugh. Too hard!

@jhmatthews
Copy link
Collaborator

We can discuss in person I guess. Perhaps I'm being a little unnecessarily pernickety here, sorry. Also, I figured out the problem with domain2, so that's all good 😄

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