rjkoch
commented
4 years ago @pavoljuhas @sbillinge hold off on review until I can get the Travis CI build to run ok.
Closed rjkoch closed 4 years ago
rjkoch
commented
4 years ago @pavoljuhas @sbillinge hold off on review until I can get the Travis CI build to run ok.
rjkoch
commented
4 years ago @pavoljuhas @sbillinge Ready for review.
codecov[bot]
commented
4 years ago Merging #30 into master will decrease coverage by
0.22%. The diff coverage is100%.
@@ Coverage Diff @@
## master #30 +/- ##
==========================================
- Coverage 88.61% 88.39% -0.23%
==========================================
Files 138 137 -1
Lines 7508 6615 -893
Branches 524 472 -52
==========================================
- Hits 6653 5847 -806
+ Misses 659 550 -109
- Partials 196 218 +22| Impacted Files | Coverage Δ | |
|---|---|---|
| src/diffpy/srreal/JeongPeakWidth.hpp | 100% <ø> (ø) |
:arrow_up: |
| src/tests/TestPeakWidthModel.hpp | 100% <100%> (ø) |
:arrow_up: |
| src/diffpy/srreal/JeongPeakWidth.cpp | 100% <100%> (ø) |
:arrow_up: |
| src/tests/TestPDFCalculator.hpp | 100% <100%> (ø) |
:arrow_up: |
| src/diffpy/srreal/PDFBaseline.hpp | 0% <0%> (-100%) |
:arrow_down: |
| src/diffpy/srreal/BaseBondGenerator.hpp | 0% <0%> (-100%) |
:arrow_down: |
| src/diffpy/srreal/AtomRadiiTable.hpp | 50% <0%> (-50%) |
:arrow_down: |
| src/diffpy/HasClassRegistry.hpp | 66.66% <0%> (-33.34%) |
:arrow_down: |
| src/diffpy/srreal/PQEvaluator.hpp | 66.66% <0%> (-33.34%) |
:arrow_down: |
| src/diffpy/mathutils.ipp | 54.54% <0%> (-26.94%) |
:arrow_down: |
| ... and 97 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 1ac3dcf...7dee219. Read the comment docs.
rjkoch
commented
4 years ago I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge notifications@github.com wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484:
@@ -30,14 +30,16 @@ using namespace std; // Constructors --------------------------------------------------------------
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJHXSEL7PFK53DINW5LQXN6WHA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOKUYGY#pullrequestreview-328551451, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA .
sbillinge
commented
4 years ago Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch notifications@github.com wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge notifications@github.com wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484:
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJHXSEL7PFK53DINW5LQXN6WHA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOKUYGY#pullrequestreview-328551451 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWULYI7RKX5LNTTCXZRLQXOK77A5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGGDKQ#issuecomment-562848170, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA .
rjkoch
commented
4 years ago Cool, I'll do that.
So, I can't actually get this to build on my system, mostly because I've never built nor worked on C++ (hence I had to see Travis fail to know it wouldn't build). Maybe there is a way for me to test it prior to merging it in?
On Sat, Dec 7, 2019, 9:13 AM Simon Billinge notifications@github.com wrote:
Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch notifications@github.com wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge notifications@github.com wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484:
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWULYI7RKX5LNTTCXZRLQXOK77A5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGGDKQ#issuecomment-562848170 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJEYAE5HCV6BLXIKNQDQXOVSLA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGHWSI#issuecomment-562854729, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMSFZJCSATTKUFUYWQKF3JLQXOVSLANCNFSM4JV4QYAA .
sbillinge
commented
4 years ago I don't know much about that. Pavol or someone who knows some C would be needed. Does Traveis compile the c-code as well as running the tests?
On Sat, Dec 7, 2019 at 9:37 AM Robert J Koch notifications@github.com wrote:
Cool, I'll do that.
So, I can't actually get this to build on my system, mostly because I've never built nor worked on C++ (hence I had to see Travis fail to know it wouldn't build). Maybe there is a way for me to test it prior to merging it in?
On Sat, Dec 7, 2019, 9:13 AM Simon Billinge notifications@github.com wrote:
Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch notifications@github.com wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge notifications@github.com wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484:
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJEYAE5HCV6BLXIKNQDQXOVSLA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGHWSI#issuecomment-562854729 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AMSFZJCSATTKUFUYWQKF3JLQXOVSLANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUP6TLQG4DZ7OPBZ7MLQXOYKNA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGID4I#issuecomment-562856433, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUMWN4WKD3XBQLJD5TTQXOYKNANCNFSM4JV4QYAA .
rjkoch
commented
4 years ago It does build them in 5 environments. Maybe I can pull the built packages from there. I did modify the tests to include the new parameter, and they pass, but it would be good to see the PDF.
I started doing this in Pdfgui, but it's a bit more involved.
On Sat, Dec 7, 2019, 9:39 AM Simon Billinge notifications@github.com wrote:
I don't know much about that. Pavol or someone who knows some C would be needed. Does Traveis compile the c-code as well as running the tests?
On Sat, Dec 7, 2019 at 9:37 AM Robert J Koch notifications@github.com wrote:
Cool, I'll do that.
So, I can't actually get this to build on my system, mostly because I've never built nor worked on C++ (hence I had to see Travis fail to know it wouldn't build). Maybe there is a way for me to test it prior to merging it in?
On Sat, Dec 7, 2019, 9:13 AM Simon Billinge notifications@github.com wrote:
Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch <notifications@github.com
wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge < notifications@github.com> wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp <https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484 :
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJCSATTKUFUYWQKF3JLQXOVSLANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUP6TLQG4DZ7OPBZ7MLQXOYKNA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGID4I#issuecomment-562856433 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABAOWUMWN4WKD3XBQLJD5TTQXOYKNANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJHOAWQEFMRMFP33SUDQXOYRDA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIE4Q#issuecomment-562856562, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMSFZJH37Z333DYDCCGOWCTQXOYRDANCNFSM4JV4QYAA .
sbillinge
commented
4 years ago can you look at the travis build and see how it builds it and then try and replicate that locally? I am ok with merging it for testing, but I guess you would still have to build it locally? Not sure how to pull built packages from travis tbh
On Sat, Dec 7, 2019 at 9:42 AM Robert J Koch notifications@github.com wrote:
It does build them in 5 environments. Maybe I can pull the built packages from there. I did modify the tests to include the new parameter, and they pass, but it would be good to see the PDF.
I started doing this in Pdfgui, but it's a bit more involved.
On Sat, Dec 7, 2019, 9:39 AM Simon Billinge notifications@github.com wrote:
I don't know much about that. Pavol or someone who knows some C would be needed. Does Traveis compile the c-code as well as running the tests?
On Sat, Dec 7, 2019 at 9:37 AM Robert J Koch notifications@github.com wrote:
Cool, I'll do that.
So, I can't actually get this to build on my system, mostly because I've never built nor worked on C++ (hence I had to see Travis fail to know it wouldn't build). Maybe there is a way for me to test it prior to merging it in?
On Sat, Dec 7, 2019, 9:13 AM Simon Billinge notifications@github.com wrote:
Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch < notifications@github.com
wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge < notifications@github.com> wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp < https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484 :
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJCSATTKUFUYWQKF3JLQXOVSLANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUMWN4WKD3XBQLJD5TTQXOYKNANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJHOAWQEFMRMFP33SUDQXOYRDA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIE4Q#issuecomment-562856562 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AMSFZJH37Z333DYDCCGOWCTQXOYRDANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUNDPYRTKYMSQRBYPQLQXOY5HA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIG3I#issuecomment-562856813, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUPNV2QYAHZVBGVJ2B3QXOY5HANCNFSM4JV4QYAA .
rjkoch
commented
4 years ago Yeah, I could probably look at Travis to see how it builds. The documentation states clearly how to build it, and I'm getting errors on linking boost libraries, but it's not a a straight forward fix because it's a C++ package being built by a Python module (scons)...
I don't think I would have to build it locally. When it gets installed as a Python package through conda as part of CMI I certainly don't do any building. It just works.
On Sat, Dec 7, 2019, 9:45 AM Simon Billinge notifications@github.com wrote:
can you look at the travis build and see how it builds it and then try and replicate that locally? I am ok with merging it for testing, but I guess you would still have to build it locally? Not sure how to pull built packages from travis tbh
On Sat, Dec 7, 2019 at 9:42 AM Robert J Koch notifications@github.com wrote:
It does build them in 5 environments. Maybe I can pull the built packages from there. I did modify the tests to include the new parameter, and they pass, but it would be good to see the PDF.
I started doing this in Pdfgui, but it's a bit more involved.
On Sat, Dec 7, 2019, 9:39 AM Simon Billinge notifications@github.com wrote:
I don't know much about that. Pavol or someone who knows some C would be needed. Does Traveis compile the c-code as well as running the tests?
On Sat, Dec 7, 2019 at 9:37 AM Robert J Koch <notifications@github.com
wrote:
Cool, I'll do that.
So, I can't actually get this to build on my system, mostly because I've never built nor worked on C++ (hence I had to see Travis fail to know it wouldn't build). Maybe there is a way for me to test it prior to merging it in?
On Sat, Dec 7, 2019, 9:13 AM Simon Billinge < notifications@github.com> wrote:
Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch < notifications@github.com
wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge < notifications@github.com> wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp < https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484 :
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJCSATTKUFUYWQKF3JLQXOVSLANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUMWN4WKD3XBQLJD5TTQXOYKNANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJH37Z333DYDCCGOWCTQXOYRDANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUNDPYRTKYMSQRBYPQLQXOY5HA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIG3I#issuecomment-562856813 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABAOWUPNV2QYAHZVBGVJ2B3QXOY5HANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJEH232ELHZ5QUS3QBTQXOZHRA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIIRA#issuecomment-562857028, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMSFZJBRSG2L4UNZBTI2QLDQXOZHRANCNFSM4JV4QYAA .
sbillinge
commented
4 years ago right. The packages on conda though have gone through a full release cycle. This either takes a Travis built version and makes a tarball, or else there is some compiling that Pavol does to build it before it is released, I am not sure which. Maybe reach out to Tim who has gone through a full release of pdfgetx3 with instructions from Pavol, though I think that may be pure python so we may need the same kind of help from Pavol for this one. Obviously we don't want to release it to conda until we are sure it is building and running successfully....
On Sat, Dec 7, 2019 at 9:55 AM Robert J Koch notifications@github.com wrote:
Yeah, I could probably look at Travis to see how it builds. The documentation states clearly how to build it, and I'm getting errors on linking boost libraries, but it's not a a straight forward fix because it's a C++ package being built by a Python module (scons)...
I don't think I would have to build it locally. When it gets installed as a Python package through conda as part of CMI I certainly don't do any building. It just works.
On Sat, Dec 7, 2019, 9:45 AM Simon Billinge notifications@github.com wrote:
can you look at the travis build and see how it builds it and then try and replicate that locally? I am ok with merging it for testing, but I guess you would still have to build it locally? Not sure how to pull built packages from travis tbh
On Sat, Dec 7, 2019 at 9:42 AM Robert J Koch notifications@github.com wrote:
It does build them in 5 environments. Maybe I can pull the built packages from there. I did modify the tests to include the new parameter, and they pass, but it would be good to see the PDF.
I started doing this in Pdfgui, but it's a bit more involved.
On Sat, Dec 7, 2019, 9:39 AM Simon Billinge notifications@github.com wrote:
I don't know much about that. Pavol or someone who knows some C would be needed. Does Traveis compile the c-code as well as running the tests?
On Sat, Dec 7, 2019 at 9:37 AM Robert J Koch < notifications@github.com
wrote:
Cool, I'll do that.
So, I can't actually get this to build on my system, mostly because I've never built nor worked on C++ (hence I had to see Travis fail to know it wouldn't build). Maybe there is a way for me to test it prior to merging it in?
On Sat, Dec 7, 2019, 9:13 AM Simon Billinge < notifications@github.com> wrote:
Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch < notifications@github.com
wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge < notifications@github.com> wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp < https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484 :
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJCSATTKUFUYWQKF3JLQXOVSLANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUMWN4WKD3XBQLJD5TTQXOYKNANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJH37Z333DYDCCGOWCTQXOYRDANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUPNV2QYAHZVBGVJ2B3QXOY5HANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJEH232ELHZ5QUS3QBTQXOZHRA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIIRA#issuecomment-562857028 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AMSFZJBRSG2L4UNZBTI2QLDQXOZHRANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUO6JCWJTB3UXWCMM7TQXO2NXA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIPFQ#issuecomment-562857878, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUKA3XTLXYAJTRPTL3TQXO2NXANCNFSM4JV4QYAA .
rjkoch
commented
4 years ago Yeah I agree we need to test before release. I'll see about either figuring out why my build isn't running or pulling from Travis to test. If I can't manage I'll reach out to Tim.
On Sat, Dec 7, 2019, 10:01 AM Simon Billinge notifications@github.com wrote:
right. The packages on conda though have gone through a full release cycle. This either takes a Travis built version and makes a tarball, or else there is some compiling that Pavol does to build it before it is released, I am not sure which. Maybe reach out to Tim who has gone through a full release of pdfgetx3 with instructions from Pavol, though I think that may be pure python so we may need the same kind of help from Pavol for this one. Obviously we don't want to release it to conda until we are sure it is building and running successfully....
On Sat, Dec 7, 2019 at 9:55 AM Robert J Koch notifications@github.com wrote:
Yeah, I could probably look at Travis to see how it builds. The documentation states clearly how to build it, and I'm getting errors on linking boost libraries, but it's not a a straight forward fix because it's a C++ package being built by a Python module (scons)...
I don't think I would have to build it locally. When it gets installed as a Python package through conda as part of CMI I certainly don't do any building. It just works.
On Sat, Dec 7, 2019, 9:45 AM Simon Billinge notifications@github.com wrote:
can you look at the travis build and see how it builds it and then try and replicate that locally? I am ok with merging it for testing, but I guess you would still have to build it locally? Not sure how to pull built packages from travis tbh
On Sat, Dec 7, 2019 at 9:42 AM Robert J Koch <notifications@github.com
wrote:
It does build them in 5 environments. Maybe I can pull the built packages from there. I did modify the tests to include the new parameter, and they pass, but it would be good to see the PDF.
I started doing this in Pdfgui, but it's a bit more involved.
On Sat, Dec 7, 2019, 9:39 AM Simon Billinge < notifications@github.com> wrote:
I don't know much about that. Pavol or someone who knows some C would be needed. Does Traveis compile the c-code as well as running the tests?
On Sat, Dec 7, 2019 at 9:37 AM Robert J Koch < notifications@github.com
wrote:
Cool, I'll do that.
So, I can't actually get this to build on my system, mostly because I've never built nor worked on C++ (hence I had to see Travis fail to know it wouldn't build). Maybe there is a way for me to test it prior to merging it in?
On Sat, Dec 7, 2019, 9:13 AM Simon Billinge < notifications@github.com> wrote:
Yup, sounds good to me
On Sat, Dec 7, 2019 at 3:43 PM Robert J Koch < notifications@github.com
wrote:
I'm not devoted to the name as it stands. It can be refactored quite easily. My main concern is accuracy and functionality.
What about appending _seperable instead of _new?
On Sat, Dec 7, 2019, 5:58 AM Simon Billinge < notifications@github.com> wrote:
@sbillinge commented on this pull request.
one small but I think important comment on naming variables
In src/diffpy/srreal/JeongPeakWidth.cpp < https://github.com/diffpy/libdiffpy/pull/30#discussion_r355115484 :
@@ -30,14 +30,16 @@ using namespace std; // Constructors
JeongPeakWidth::JeongPeakWidth() :
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0)
- mdelta1(0.0), mdelta2(0.0), mqbroad(0.0), mqbroad_new(0.0)
I think this is not a good name. A more descriptive name would be better. I don't know, separable_mqbroad or something along those lines?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJDPBSFPHHQ74WQCBGLQXN6WHANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUKZ7X3DTZFR6CUG3XDQXOK77ANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJCSATTKUFUYWQKF3JLQXOVSLANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUMWN4WKD3XBQLJD5TTQXOYKNANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJH37Z333DYDCCGOWCTQXOYRDANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUPNV2QYAHZVBGVJ2B3QXOY5HANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJBRSG2L4UNZBTI2QLDQXOZHRANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUO6JCWJTB3UXWCMM7TQXO2NXA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIPFQ#issuecomment-562857878 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABAOWUKA3XTLXYAJTRPTL3TQXO2NXANCNFSM4JV4QYAA
.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJF6QG6CRHBLF57WQ5TQXO3GLA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGIT2Q#issuecomment-562858474, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMSFZJABUNK5RNCLGPOKFWDQXO3GLANCNFSM4JV4QYAA .
rjkoch
commented
4 years ago I've renamed the variable as per our conversation.
I've also learned that Travis can provide a draft release, by placing the following in the .travis.yml file:
deploy:
provider: releases
# ⋮
draft: trueI've modified this file and am pushing as we speak,. Fingers crossed.
rjkoch
commented
4 years ago More on draft releases are here: https://docs.travis-ci.com/user/deployment/releases/#draft-releases-with-draft-true
sbillinge
commented
4 years ago @rjkoch I merged this so we can test the release etc, but we will need docs for sure. this is a major change in functionality. Maybe even a paper? I am interested to see if this is more stable than the original Qbroad which I never felt was very stable.
rjkoch
commented
4 years ago Ok, I have beamtime this week till Thursday, but I'll work on this when I can.
I definitely am interesting in testing it, and can start on documentation. If it works well and is more stable, I think a paper would definitely be in order. I don't think the science is necessarily new, as I know some codes are already using such an implementation, but this new functionality in CMI is definitely worth putting out there.
On Sun, Dec 8, 2019 at 11:50 PM Simon Billinge notifications@github.com wrote:
@rjkoch https://github.com/rjkoch I merged this so we can test the release etc, but we will need docs for sure. this is a major change in functionality. Maybe even a paper? I am interested to see if this is more stable than the original Qbroad which I never felt was very stable.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJA6JJ23KQXJNLAJSTDQXXFBZA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGH2HXY#issuecomment-563061727, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMSFZJARL55SGUZYBQTY3DDQXXFBZANCNFSM4JV4QYAA .
sbillinge
commented
4 years ago Yes a code release will be good for the community. The paper would just be comparing the two implementations since so many have used the old one. and advertising which is better. It should be short and sweet (and the testing also). I wasn't actually aware that it was implemented wrong to be honest, it probably dates back to Thomas and PDFFIT. Actually, strictly there is no "wrong" or "right" because it is an empirical correction with no scientific basis for the functional form, but there will definitely be better and worse implementations and I think the separable one is almost certainly going to be better, for all the reasons you gave, not to mention the fact that I was never satisfied with the stability and reproducibility of the old one.
All in all, worth a short paper I think, if we can find the time.
S
On Mon, Dec 9, 2019 at 9:49 AM Robert J Koch notifications@github.com wrote:
Ok, I have beamtime this week till Thursday, but I'll work on this when I can.
I definitely am interesting in testing it, and can start on documentation. If it works well and is more stable, I think a paper would definitely be in order. I don't think the science is necessarily new, as I know some codes are already using such an implementation, but this new functionality in CMI is definitely worth putting out there.
On Sun, Dec 8, 2019 at 11:50 PM Simon Billinge notifications@github.com wrote:
@rjkoch https://github.com/rjkoch I merged this so we can test the release etc, but we will need docs for sure. this is a major change in functionality. Maybe even a paper? I am interested to see if this is more stable than the original Qbroad which I never felt was very stable.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJA6JJ23KQXJNLAJSTDQXXFBZA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGH2HXY#issuecomment-563061727 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AMSFZJARL55SGUZYBQTY3DDQXXFBZANCNFSM4JV4QYAA
.
— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUK74O3GM7YS5JVHDIDQXZLG3A5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGJNWXI#issuecomment-563272541, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUOGA33ITMQZZ7SBCITQXZLG3ANCNFSM4JV4QYAA .
rjkoch
commented
4 years ago Did something else change this morning? Initially, things looked good, but now the Travis page for this library (and indeed all the diffpy libraries....) are down..?
On Mon, Dec 9, 2019 at 10:07 AM Simon Billinge notifications@github.com wrote:
Yes a code release will be good for the community. The paper would just be comparing the two implementations since so many have used the old one. and advertising which is better. It should be short and sweet (and the testing also). I wasn't actually aware that it was implemented wrong to be honest, it probably dates back to Thomas and PDFFIT. Actually, strictly there is no "wrong" or "right" because it is an empirical correction with no scientific basis for the functional form, but there will definitely be better and worse implementations and I think the separable one is almost certainly going to be better, for all the reasons you gave, not to mention the fact that I was never satisfied with the stability and reproducibility of the old one.
All in all, worth a short paper I think, if we can find the time.
S
On Mon, Dec 9, 2019 at 9:49 AM Robert J Koch notifications@github.com wrote:
Ok, I have beamtime this week till Thursday, but I'll work on this when I can.
I definitely am interesting in testing it, and can start on documentation. If it works well and is more stable, I think a paper would definitely be in order. I don't think the science is necessarily new, as I know some codes are already using such an implementation, but this new functionality in CMI is definitely worth putting out there.
On Sun, Dec 8, 2019 at 11:50 PM Simon Billinge <notifications@github.com
wrote:
@rjkoch https://github.com/rjkoch I merged this so we can test the release etc, but we will need docs for sure. this is a major change in functionality. Maybe even a paper? I am interested to see if this is more stable than the original Qbroad which I never felt was very stable.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJARL55SGUZYBQTY3DDQXXFBZANCNFSM4JV4QYAA
.
— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUK74O3GM7YS5JVHDIDQXZLG3A5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGJNWXI#issuecomment-563272541 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/ABAOWUOGA33ITMQZZ7SBCITQXZLG3ANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJE7QDLELZ6KFFLUOFTQXZNLZA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGJP4UQ#issuecomment-563281490, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMSFZJEK4MYUUUXQZP2IWM3QXZNLZANCNFSM4JV4QYAA .
sbillinge
commented
4 years ago I didn't do anything. Let me take a look
S
On Mon, Dec 9, 2019 at 1:44 PM Robert J Koch notifications@github.com wrote:
Did something else change this morning? Initially, things looked good, but now the Travis page for this library (and indeed all the diffpy libraries....) are down..?
On Mon, Dec 9, 2019 at 10:07 AM Simon Billinge notifications@github.com wrote:
Yes a code release will be good for the community. The paper would just be comparing the two implementations since so many have used the old one. and advertising which is better. It should be short and sweet (and the testing also). I wasn't actually aware that it was implemented wrong to be honest, it probably dates back to Thomas and PDFFIT. Actually, strictly there is no "wrong" or "right" because it is an empirical correction with no scientific basis for the functional form, but there will definitely be better and worse implementations and I think the separable one is almost certainly going to be better, for all the reasons you gave, not to mention the fact that I was never satisfied with the stability and reproducibility of the old one.
All in all, worth a short paper I think, if we can find the time.
S
On Mon, Dec 9, 2019 at 9:49 AM Robert J Koch notifications@github.com wrote:
Ok, I have beamtime this week till Thursday, but I'll work on this when I can.
I definitely am interesting in testing it, and can start on documentation. If it works well and is more stable, I think a paper would definitely be in order. I don't think the science is necessarily new, as I know some codes are already using such an implementation, but this new functionality in CMI is definitely worth putting out there.
On Sun, Dec 8, 2019 at 11:50 PM Simon Billinge < notifications@github.com
wrote:
@rjkoch https://github.com/rjkoch I merged this so we can test the release etc, but we will need docs for sure. this is a major change in functionality. Maybe even a paper? I am interested to see if this is more stable than the original Qbroad which I never felt was very stable.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AMSFZJARL55SGUZYBQTY3DDQXXFBZANCNFSM4JV4QYAA
.
— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <
, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ABAOWUOGA33ITMQZZ7SBCITQXZLG3ANCNFSM4JV4QYAA
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=AMSFZJE7QDLELZ6KFFLUOFTQXZNLZA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGJP4UQ#issuecomment-563281490 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AMSFZJEK4MYUUUXQZP2IWM3QXZNLZANCNFSM4JV4QYAA
.
— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/diffpy/libdiffpy/pull/30?email_source=notifications&email_token=ABAOWUNSY3ZBJCIORIXMKETQX2GXZA5CNFSM4JV4QYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGKGXLA#issuecomment-563375020, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUPNZAFRQS6S3L5M2OLQX2GXZANCNFSM4JV4QYAA .
sbillinge
commented
4 years ago This PR is merged....which is why there is no travis build. You can git pull upstream master and run tests locally I guess?
…as per https://doi.org/10.1107/S0108767391011327
Should resolve #29, but may require modification of diffpy.srreal package.
@pavoljuhas @sbillinge Ready for review