Fitter improvement #198

weiliangjin2021 · 2022-02-03T23:20:21Z

More error handling in StableDispersiveFitter
Allow to load dispersive data directly by providing URL to txt or csv file
Add tunable eps_inf as a variable for optimization
Allow to set frequency range for fitting

tylerflex · 2022-02-03T23:58:24Z

tests/test_fit_url.py

+import numpy as np
+import sys
+
+sys.path.append("../tidy3d_client_revamp")


we shouldn't have this ideally. In tests/init.py we have the importing of the tidy3d package.
https://github.com/flexcompute/tidy3d/blob/develop/tests/__init__.py#L3
so can you see if removing this line still works? also the name of the repo has changed.

It is now removed, and it is still working.

tylerflex · 2022-02-03T23:59:16Z

tidy3d/plugins/dispersion/fit.py

+        self.n_data = self.n_data[ind_final]
+        self.k_data = self.k_data[ind_final]
+
+        if np.all(ind_final) is False:


I think we can do if not np.all(...)

tylerflex · 2022-02-03T23:59:53Z

tidy3d/plugins/dispersion/fit.py

+            The beginning of wavelength range. Unit: micron
+        wvl_max : float
+            The end of wavelength range. Unit: micron
+        """


what does this return?

tylerflex · 2022-02-04T00:00:39Z

tidy3d/plugins/dispersion/fit.py

+            log.error("No data within [wvl_min,wvl_max]")
+            return
+
+        self.__init__(self.wvl_um, self.n_data, self.k_data)


is this supposed to return? I'm confused

I suppose it just updates wvl_um, n_data, etc., but doesn't return anything.

aren't the values updated above? is the call to init needed?

there are several others to be updated, such as frequency_range, eps_data, etc.

tylerflex · 2022-02-04T00:01:08Z

tidy3d/plugins/dispersion/fit.py

+    def validate_url_load(data_load: List):
+        """Validate if the loaded data from URL is valid
+            The data list should be in this format:
+                [["wl",     "n"],


can we see whether this looks ok in the docs?

in the from_url, now I format it as example

tylerflex · 2022-02-04T00:02:03Z

tidy3d/plugins/dispersion/fit.py

+        url_file : str
+            Url link to the csv file.
+            e.g. "https://refractiveindex.info/data_csv.php?datafile=data/main/Ag/Johnson.yml"
+        delimiter : str, optional


instead of str, optional, we do str = ","

tylerflex · 2022-02-04T00:03:21Z

tidy3d/plugins/dispersion/fit.py

+            cls.validate_url_load(data_url)
+        except ValidationError as e:  # pylint:disable=unused-variable
+            # log.error(e)
+            return obj_failed


what is this case? maybe we should just raise a Tidy3D exception from e?

now all they are all handled by Tidy3D exception

tylerflex

see comments, also let's put some lines in the CHANGELOG.md explaining the new features.

tylerflex

Let's do the final things to this and then wrap it up:

Make these 6 functions at the beginning of the file (_unpack_complex, etc) into @staticmethod in DispersionFitter, change how they are called inside the code to add self. where necessary.
Change DispersionFitter to a pydantic.BaseModel object. This will be good practice.
a. wvl_um, n_data, k_data will be defined using pydantic.Field(). Can remove init method and docstring entirely.
b. _validate_data can be written as a pydantic.validator (see other examples in code).
c. k_data can also be handled using a pydantic.validator that ha the side effect of setting k_data if it's None.
d. Make self.eps_data, self.lossy, self.freqs, self.frequency_range into @property types. Then if things are changed (someone sets the wvl_range, or changes k_data), these things will be updated.

I think this will be a few changes but will ultimately be much cleaner and I think it will be ready after this. Also it will get documented fine.

tylerflex · 2022-02-08T22:08:22Z

tidy3d/plugins/dispersion/fit_web.py

-        nlopt_maxeval : PositiveInt, optional
-            Maxeval in each optimization
+        advanced_param : AdvancedFitterParam, optional
+            Other advanced parameters.


let's format this like

:class:`AdvancedFitterParam`

so it links in the docs.

All revised now.

tylerflex · 2022-02-08T22:08:46Z

tidy3d/plugins/dispersion/fit_web.py

+        try:
+            resp.raise_for_status()
+        except Exception as e:
+            log.error(e)


we can remove this, when you raise a WebError it will log as well

It has been updated.

tylerflex · 2022-02-08T22:09:18Z

tidy3d/plugins/dispersion/fit_web.py

+            log.error(e)
+            raise WebError(  # pylint:disable=raise-missing-from
+                "Authorization to the server failed. Please try again."
+            )


we can raise WebError(...) from e to include the original exception.

tylerflex

A few minor comments and we can merge PR

tylerflex · 2022-02-14T23:12:25Z

tests/_test_fit_web.py

 if __name__ == "__main__":
-    test_dispersion_load_file()


let's delete this entire code block unless we need it.

tylerflex · 2022-02-14T23:12:37Z

tests/_test_fit_web.py

+    num_poles = 2
+    num_tries = 50
+    tolerance_rms = 1e-3
+    # best_medium, best_rms = fitter.fit(


remove comments unless needed.

tylerflex · 2022-02-14T23:13:44Z

tidy3d/plugins/dispersion/fit_web.py

 from .fit import DispersionFitter


-class FitterData(BaseModel):
+class AdvancedFitterParam(BaseModel):
+


hm, probably shouldn't be a space here? Does black not re-format this?

tylerflex · 2022-02-14T23:15:11Z

tidy3d/plugins/dispersion/fit_web.py

+        "hard",
+        title="Type of constraint for stability",
+        description="Stability constraint that is hard but fast to compute, "
+        "or soft but slightly slower to compute.",


when would user want "soft"? Could you explain just a little more? Seems like soft is less stable and slower to compute? but more likely to return a fit?

tylerflex · 2022-02-14T23:15:24Z

tidy3d/plugins/dispersion/fit_web.py

+    nlopt_maxeval: PositiveInt = Field(
+        5000,
+        title="Number of inner iterations",
+        description="Number of iterations in each inner optimization",


period at end of description

tylerflex · 2022-02-14T23:17:10Z

tidy3d/plugins/dispersion/fit_web.py

+            Other advanced parameters.
+
+        Returns
+        -------
        Tuple[``PoleResidue``, float]


:class:`PoleResiude`

tylerflex · 2022-02-14T23:18:32Z

tidy3d/plugins/dispersion/fit_web.py

-        _url = self._set_url("default")
+        # set up bound_f, bound_amp
+        if advanced_param.bound_f is None:
+            advanced_param.bound_f = self.frequency_range[1] * 10


instead of hardcoding this 10, let's make it either a constant BOUND_MAX_FACTOR = 10 defined somewhere or an argument to this function if it makes more sense.

tylerflex · 2022-02-14T23:18:41Z

tidy3d/plugins/dispersion/fit_web.py

+        if advanced_param.bound_f is None:
+            advanced_param.bound_f = self.frequency_range[1] * 10
+        if advanced_param.bound_amp is None:
+            advanced_param.bound_amp = self.frequency_range[1] * 10


see comment above

Change DispersionFitter to a pydantic.BaseModel object. Make eps_data, lossy, freqs, frequency_range into @Property types More error handling in StableDispersiveFitter Allow to load dispersive data directly by providing URL to txt or csv file Allow to filter wavelength range for fitting Add tunable eps_inf as a variable for optimization

weiliangjin2021 · 2022-02-15T01:03:17Z

All the above comments have been addressed in this commit

weiliangjin2021 requested a review from twhughes February 3, 2022 23:20

tylerflex reviewed Feb 3, 2022

View reviewed changes

tylerflex reviewed Feb 4, 2022

View reviewed changes

tylerflex requested changes Feb 4, 2022

View reviewed changes

flexcompute deleted a comment from twhughes Feb 4, 2022

weiliangjin2021 force-pushed the weiliang/url_web branch 3 times, most recently from b6e470b to c34ed59 Compare February 7, 2022 19:13

tylerflex requested changes Feb 8, 2022

View reviewed changes

weiliangjin2021 force-pushed the weiliang/url_web branch 2 times, most recently from 2c40af8 to 058cc88 Compare February 14, 2022 23:11

tylerflex requested changes Feb 14, 2022

View reviewed changes

weiliangjin2021 force-pushed the weiliang/url_web branch from 058cc88 to 74c5eb6 Compare February 15, 2022 01:02

tylerflex approved these changes Feb 15, 2022

View reviewed changes

fixed docstrings

c3f21e7

tylerflex merged commit 344c7e6 into develop Feb 15, 2022

tylerflex deleted the weiliang/url_web branch March 25, 2022 02:01

Fitter improvement #198

Fitter improvement #198

Conversation

weiliangjin2021 commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerflex Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerflex left a comment

Choose a reason for hiding this comment

tylerflex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiliangjin2021 Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

tylerflex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiliangjin2021 commented Feb 15, 2022

tylerflex Feb 4, 2022 •

edited

Loading

weiliangjin2021 Feb 14, 2022 •

edited

Loading