oblique by lisdanil · Pull Request #48 · hombit/freddi

lisdanil · 2021-10-29T19:50:32Z

No description provided.

hombit

Good work!

I don't like having both R_Alfven and R_Alfven_basic methods, do we really need both these numbers in the code?
I don't think that truncateInnerRadius is a good place to check Rm_definition value, probably we need some abstract class with two child classes to implement Rm calculation and use dynamic dispatch to actually get it's value. It would be much faster and more idiomatic. I know that you can find this "bad" snippets in the code but they are legacy and I wouldn't introduce this into new code.
Please fix git merge conflict
Please remove freddi binary

hombit · 2021-11-08T21:09:33Z

cpp/src/ns/ns_evolution.cpp

+
+
+double FreddiNeutronStarEvolution::R_Magn_bozzo18() const {
+	const double chi_oblique_rad = double(chioblique()) * M_PI / 180.;


Why do you need this double() casting here?

hombit · 2021-11-08T21:11:44Z

cpp/src/ns/ns_evolution.cpp

+
+double FreddiNeutronStarEvolution::R_Magn_bozzo18() const {
+	const double chi_oblique_rad = double(chioblique()) * M_PI / 180.;
+    const double alpha = 0.5; // как его взять честно??? его надо откуда-то вызвать


args().basic->alpha

cpp/src/ns/ns_evolution.cpp

hombit · 2021-11-08T21:14:28Z

cpp/src/ns/ns_evolution.cpp

+    //const vecd& H = Height();//почему так нельзя?
+    const double H_to_R_temp = h2rbozzo(); //временно


const vecd& H = Height(); is ok in terms of C++ but Freddi has non-physical vertical structure for inner part of the disc

hombit · 2021-11-08T21:15:38Z

cpp/src/ns/ns_evolution.cpp

+
+    //const vecd& H = Height();//почему так нельзя?
+    const double H_to_R_temp = h2rbozzo(); //временно
+	const bool sign_at_inf = 1; //sign on unfunuty should be minus


I don't like implicit integer to boolean type casting here

hombit · 2021-11-08T21:17:55Z

cpp/src/ns/ns_evolution.cpp

+    const double xbozzo_0 = R()[first()] / RCorrot;
+    const double fxbozzo_0 = parametr_bozzo * ( (1. - std::pow(xbozzo_0, 1.5) ) * m::pow<2>(std::cos(chi_oblique_rad)) + H_to_R_temp * (8. - 5. * std::pow(xbozzo_0, 1.5)) * m::pow<2>(std::sin(chi_oblique_rad))) - std::pow(xbozzo_0 * parametr_thetta, 3.5);
+
+    if (std::signbit(fxbozzo_0) == sign_at_inf){


Is sign_at_inf will still be compilation time constant in the future? If yes, I would prefer just writing
if (std::signbit(fxbozzo_0)){ with the proper comment

hombit · 2021-11-08T21:19:15Z

cpp/src/ns/ns_evolution.cpp

+    const double fxbozzo_0 = parametr_bozzo * ( (1. - std::pow(xbozzo_0, 1.5) ) * m::pow<2>(std::cos(chi_oblique_rad)) + H_to_R_temp * (8. - 5. * std::pow(xbozzo_0, 1.5)) * m::pow<2>(std::sin(chi_oblique_rad))) - std::pow(xbozzo_0 * parametr_thetta, 3.5);
+
+    if (std::signbit(fxbozzo_0) == sign_at_inf){
+        return 0. ; 


Does zero Rmagn have any sense?

i wrote a comment

hombit · 2021-11-08T21:23:20Z

cpp/include/ns/ns_arguments.hpp

-			double Bx, double hotspotarea,
-			double epsilonAlfven, double inversebeta, double Rdead,
+			double Bx,
+            const std::string& Rm_definition, double h2r_bozzo, double chi_oblique, 


Consider to rename to Rm_type

BTW, is it actually used?

hombit · 2021-11-08T21:24:14Z

cpp/src/ns/ns_evolution.cpp

 		return;
 	}
+    //const int Rm_definition = 1;
+    std::string Rmdef = Rmdefinition(); 


Unnecessary copy, use constant reference instead

hombit · 2021-11-08T21:26:15Z

cpp/src/ns/ns_evolution.cpp

+        }
+	}
+	if (jj == last() - 2){
+        std::cout<<"cannot find a root for R magnetosphere which is kinda weird\n";


Printing something into standard input is not enough here. If is a logical error you should raise an exception

Probably you should raise RadiusCollapseException()

hombit · 2021-11-12T19:21:01Z

R_Alfven_basic -> R_Alfven
R_Alfven -> something like R_magn

hombit

Thank you for this comment, please see comments to individual code lines

hombit · 2022-03-14T15:22:52Z

cpp/src/ns/ns_arguments.cpp


+#include <boost/math/tools/roots.hpp>
+
+#include <fstream>


Why do you need fstream here?

Fstream is here for debugging process.

hombit · 2022-03-14T15:24:10Z

cpp/src/ns/ns_arguments.cpp

+
+
+    std::pair<double, double> r = boost::math::tools::toms748_solve(
+            [RC, RA, chi_oblique_rad, parametr_bozzo, H_to_R_temp](double omega) { return parametr_bozzo * (m::pow<2>(std::cos(chi_oblique_rad)) * (1 - omega) + H_to_R_temp * (11 - 8 * omega) * m::pow<2>(std::sin(chi_oblique_rad)) ) * std::pow(RA/RC, 3.5) - 0.5 * std::pow(omega, 10./3.); },


Please avoid implicit type casting: use 1.0, 11.0, 8.0, etc

Please use line width up to 120 if possible

hombit · 2022-03-14T15:26:41Z

cpp/src/ns/ns_arguments.cpp

+    const double alpha = 0.5; //поправить потом  это дело
+    const double chi_oblique_rad = chi_oblique * M_PI / 180.;
+    const double parametr_bozzo = 1. * m::pow<2>(0.2) / alpha;
+    const double H_to_R_temp = h2r_bozzo;


Why do you need this variable?

Is it about alpha or something else?

hombit · 2022-03-14T15:34:32Z

cpp/src/ns/ns_arguments.cpp

+    const double RC = std::cbrt(GM / m::pow<2>(2*M_PI * freqx));
+    const double alpha = 0.5; //поправить потом  это дело
+    const double chi_oblique_rad = chi_oblique * M_PI / 180.;
+    const double parametr_bozzo = 1. * m::pow<2>(0.2) / alpha;


Please comment this "1. * "

It has been removed.

hombit · 2022-03-14T15:35:37Z

cpp/src/ns/ns_arguments.cpp

+    boost::math::tools::eps_tolerance<double> tol(10);
+
+
+    std::pair<double, double> r = boost::math::tools::toms748_solve(


Could be const

hombit · 2022-03-14T15:50:43Z

cpp/src/ns/ns_evolution.cpp

@@ -93,6 +98,15 @@ std::shared_ptr<FreddiNeutronStarEvolution::BasicKappaT> FreddiNeutronStarEvolut

 vecd FreddiNeutronStarEvolution::NeutronStarStructure::initialize_Fmagn(FreddiEvolution* evolution) const {


Please consider reimplementing "Rmtype" processing in these three functions:

Please add "kluznyak" to the description of --Rmtype parameter in ns_options.cpp

In these function check all supported parameters explicitly, see initializeNsMdotFraction for an example

Raise an exception if "Rmtype" value is not supported

Please modify pyton test "FmagnDerivativesTestCase" to check if derivatives are correct. It is located in /python/test/test_ns.py

hombit · 2022-03-14T15:54:33Z

cpp/src/ns/ns_evolution.cpp

+//      const double R_0 = std::max(R_Magn_KR07(), R_x); не нужно сейчас
+        const double k = inverse_beta * m::pow<2>(mu_magn) / 9.;
+	    double brackets1, brackets2;
+        double Fmagn;


Please do not introduce uninitialized variables when possible

hombit · 2022-03-14T15:58:13Z

cpp/include/ns/ns_evolution.hpp

 	public:
 		NeutronStarStructure(const NeutronStarArguments& args_ns, FreddiEvolution* evolution);
+        double R_Magn_KR07(FreddiEvolution* evolution) const;
+        double F_Magn_KR07(const double R) const;


You don't need const double specifier here, just double is fine

hombit · 2022-03-14T16:06:47Z

cpp/src/ns/ns_evolution.cpp

+
+    if (Rmtype() == "Kluzniak" || Rmtype() == "kluzniak"){
+        R_magnetic = std::max(R_Magn_KR07(), R_Mdot_slope_KR07());
+    }


Please check explicitly if unknown Rmtype is used and raise and exception in this case. BTW, the better solution would be implementing it as a virtual class with subclasses and using a dynamical dispatch instead of string comparison. See *KappaT classes for an example

hombit · 2022-03-14T16:08:31Z

cpp/src/ns/ns_evolution.cpp

+                new_F_in = kappa_t(R_m) * m::pow<2>(mu_magn()) / m::pow<3>(R_m);
+            }
+        } else {
+            new_F_in = 0;


Why do we need it now? Was it broken before?

lisdanil added 4 commits October 29, 2021 11:49

Oblique change

0b846f7

oblique

9d63e8d

Merge branch 'dev' of https://github.com/lisdanil/freddi into dev

e6ba3cd

variables added

ca12b73

hombit self-requested a review November 8, 2021 17:06

hombit requested changes Nov 8, 2021

View reviewed changes

lisdanil added 2 commits February 26, 2022 23:43

Strong magnetic field

f3f8891

Strong magnetic field (but there are some repeating functions)

47c285c

hombit requested changes Mar 14, 2022

View reviewed changes

lisdanil added 5 commits March 24, 2022 14:39

Fixed Fmagn_KR07_oblique

d3eb91d

Fmagn, dF2magn_dh2 is fixed (Kluzniak's case)

42972cc

Copy of function is fixed

e3a2f36

Without doppelgangers

fea1132

New iterating scheme

231399e



		double FreddiNeutronStarEvolution::R_Magn_bozzo18() const {
		const double chi_oblique_rad = double(chioblique()) * M_PI / 180.;

		//const vecd& H = Height();//почему так нельзя?
		const double H_to_R_temp = h2rbozzo(); //временно



		std::pair<double, double> r = boost::math::tools::toms748_solve(
		[RC, RA, chi_oblique_rad, parametr_bozzo, H_to_R_temp](double omega) { return parametr_bozzo * (m::pow<2>(std::cos(chi_oblique_rad)) * (1 - omega) + H_to_R_temp * (11 - 8 * omega) * m::pow<2>(std::sin(chi_oblique_rad)) ) * std::pow(RA/RC, 3.5) - 0.5 * std::pow(omega, 10./3.); },

		boost::math::tools::eps_tolerance<double> tol(10);


		std::pair<double, double> r = boost::math::tools::toms748_solve(

		@@ -93,6 +98,15 @@ std::shared_ptr<FreddiNeutronStarEvolution::BasicKappaT> FreddiNeutronStarEvolut

		vecd FreddiNeutronStarEvolution::NeutronStarStructure::initialize_Fmagn(FreddiEvolution* evolution) const {


		#include <boost/math/tools/roots.hpp>

		#include <fstream>

Conversation

lisdanil commented Oct 29, 2021

Uh oh!

hombit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hombit commented Nov 12, 2021

Uh oh!

hombit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

hombit left a comment •

edited

Loading