mir-group / phoebe

A high-performance framework for solving phonon and electron Boltzmann equations
https://mir-group.github.io/phoebe/
MIT License
83 stars 19 forks source link

Minor fixes of compilation #202

Closed Yaraslaut closed 10 months ago

Yaraslaut commented 10 months ago

This PR fixes few issues that I encountered when trying to compile phoebe

Yaraslaut commented 10 months ago

@anjohan @jcoulter12 is there anything else i need to do in this PR ?

jcoulter12 commented 10 months ago

HI @Yaraslaut, thanks for the reminder about this! If you did want to write a git actions routine that checked a few different compilers, maybe just for building the code, to be run during a PR, that would be super helpful. If not, that's also ok and we'll just merge this very helpful PR.

Thanks, Jenny

Yaraslaut commented 10 months ago

HI @Yaraslaut, thanks for the reminder about this! If you did want to write a git actions routine that checked a few different compilers, maybe just for building the code, to be run during a PR, that would be super helpful. If not, that's also ok and we'll just merge this very helpful PR.

Thanks, Jenny

I updated this PR with additional github actions

jcoulter12 commented 10 months ago

Hi @Yaraslaut -- this looks pretty good, but I'd prefer to not run four compilers every time in our regular build and test, as this will make the tests take a super long time. Is that what you're setting up here?

I was thinking a second/separate actions that is only triggered by PR, so that we don't have Actions taking ~an hour... I think that shouldn't be too difficult, but let me know if it is and I don't understand something.

Thanks again, Jenny

Yaraslaut commented 10 months ago

Hi @Yaraslaut -- this looks pretty good, but I'd prefer to not run four compilers every time in our regular build and test, as this will make the tests take a super long time. Is that what you're setting up here?

I was thinking a second/separate actions that is only triggered by PR, so that we don't have Actions taking ~an hour... I think that shouldn't be too difficult, but let me know if it is and I don't understand something.

Thanks again, Jenny

I update actions to run tests only for gcc 10 Github allows at least 20 concurrent jobs, so all this jobs can be performed in parallel

jcoulter12 commented 10 months ago

Ok thanks, and yes, that's of course a good point about jobs running in parallel. I was just double checking and I think for public repos, there's no limitations on actions use for now.

Once these tests pass, I think I'll go ahead and merge this. If you're later trying to use Phoebe for anything and need help, just let me know!

Yaraslaut commented 10 months ago

Ok thanks, and yes, that's of course a good point about jobs running in parallel. I was just double checking and I think for public repos, there's no limitations on actions use for now.

Once these tests pass, I think I'll go ahead and merge this. If you're later trying to use Phoebe for anything and need help, just let me know!

Thanks :) I pushed once more, and i think that some tweaking will be required to make all of them work

jcoulter12 commented 10 months ago

@Yaraslaut, I'm sure you noticed (and are just otherwise occupied, which is of course fine :) ) but the checks didn't pass -- there's no rush, but I'll keep approving git actions runs when you push things until it works.

Thanks again, Jenny

Yaraslaut commented 10 months ago

@Yaraslaut, I'm sure you noticed (and are just otherwise occupied, which is of course fine :) ) but the checks didn't pass -- there's no rush, but I'll keep approving git actions runs when you push things until it works.

Thanks again, Jenny

Yes, I saw it, will fix it soon

jcoulter12 commented 10 months ago

Hm, @Yaraslaut, I can't see how to get these last four tests to run... just in case something is going wrong, would you mind making one more small commit to see if it's just some issue with github?

Or maybe somehow the regular tests that are normally run weren't included in your last attempt at the build and test?

Thanks again, Jenny

Yaraslaut commented 10 months ago

Hm, @Yaraslaut, I can't see how to get these last four tests to run... just in case something is going wrong, would you mind making one more small commit to see if it's just some issue with github?

Or maybe somehow the regular tests that are normally run weren't included in your last attempt at the build and test?

Thanks again, Jenny

@jcoulter12 This tests does not exist anymore, they are replaces with Ubuntu 22.04 ( GCC 10 OpenMP(ON/OFF) MPI(OFF/ON)) not sure why github telling that they are required or even exist

jcoulter12 commented 10 months ago

Ok I see this now, thanks for pointing out that things have just been renamed. I'm re-running all the tests, but maybe worst case I'll force this merge without those tests passing.

Jenny

jcoulter12 commented 10 months ago

Got it, I had to change the branch protection rules for the develop branch, which were set to expect those four tests by name.

Thanks again for the help on this! Jenny