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

Fix find_idx #567

Merged
merged 27 commits into from
Oct 4, 2024
Merged

Fix find_idx #567

merged 27 commits into from
Oct 4, 2024

Conversation

jinningwang
Copy link
Member

  • Fix both ModelData.find_idx() and GroupBase.find_idx()
  • Add tests on them

@jinningwang
Copy link
Member Author

jinningwang commented Sep 27, 2024

Is the version number messed? Latest document sad v1.9.1+, but release notes have been v1.9.3+

@jinningwang
Copy link
Member Author

Do we need to worry following warnings now?

tests/test_case.py::TestPlot::test_kundur_plot
  /Users/jinningwang/work/andes/andes/utils/lazyimport.py:80: DeprecationWarning: Use shutil.which instead of find_executable
    return eval(self.__imported_name__)(*args, **kwargs)

tests/test_case.py::TestCaseInit::test_pvd1_init
  /Users/jinningwang/work/andes/andes/core/model/model.py:754: ComplexWarning: Casting complex values to real discards the imaginary part
    instance.v = np.array(func(*self.s_args[name]),

tests/test_case.py::TestCaseInit::test_pvd1_init
  /Users/jinningwang/work/andes/andes/core/model/model.py:793: ComplexWarning: Casting complex values to real discards the imaginary part
    instance.v[:] = func(*self.s_args[name])

tests/test_known_good.py: 12 warnings
  /Users/jinningwang/work/miniconda3/envs/ams/lib/python3.10/site-packages/dill/_dill.py:1028: DeprecationWarning: numpy.core is deprecated and has been renamed to numpy._core. The numpy._core namespace contains private NumPy internals and its use is discouraged, as NumPy internals can change without warning in any release. In practice, most real-world usage of numpy.core is to access functionality in the public NumPy API. If that is the case, use the public NumPy API. If not, you are using NumPy internals. If you would still like to access an internal attribute, use numpy._core._multiarray_umath.
    submodule = getattr(__import__(module, None, None, [obj]), obj)

@jinningwang
Copy link
Member Author

Docker_hub integration needs attentation, https://github.com/CURENT/andes/actions/runs/11077636077/job/30783344184?pr=567

ERROR: failed to solve: python:3.9-slim: failed to resolve source metadata for docker.io/library/python:3.9-slim: failed to authorize: failed to fetch oauth token: unexpected status from GET request to https://auth.docker.io/token?scope=repository%3Alibrary%2Fpython%3Apull&service=registry.docker.io: 401 Unauthorized
Error: Docker build failed with exit code 1

@cuihantao
Copy link
Collaborator

Jinning,

The warnings can be fixed later, but can you bump up the Python image version in the GitHub actions file to fix the Docker error?

@jinningwang
Copy link
Member Author

The trouble with github action seems come from the action setup-miniconda@v3 itself:

  /home/runner/miniconda3/condabin/conda config --add channels defaults
  Warning: Error while loading conda entry point: conda-libmamba-solver (module 'libmambapy' has no attribute 'QueryFormat')

Leave it here for now.

@@ -243,31 +243,41 @@ def set(self, src: str, idx, attr, value):

return True

def find_idx(self, keys, values, allow_none=False, default=None):
def find_idx(self, keys, values, allow_none=False, default=None,
no_flatten=False):
Copy link
Member Author

@jinningwang jinningwang Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no_flatten=False introduces more ambiguity. Remove it since we don't actually need it.
Enforce the return to be a list of lists, thus the behavior is more consistent across models and groups.

@@ -243,31 +243,41 @@ def set(self, src: str, idx, attr, value):

return True

def find_idx(self, keys, values, allow_none=False, default=None):
def find_idx(self, keys, values, allow_none=False, default=None,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default=None can be kept there to be consistent with ModelData.find_idx()

@jinningwang
Copy link
Member Author

@cuihantao

I'd like to ask for your suggestions for the below issue.

Problem

sometimes the IdxParam.v can be list of lists, which results in error in System.setup() as the IdxParam.v is expected to be a flatten list.

Minimal code

>>> raw = andes.get_case("kundur/kundur.raw")
>>> dyr = andes.get_case("kundur/kundur_full.dyr")
>>> ss = andes.load(raw, addfile=dyr, setup=False)
>>> ss.GENROU.gen.v
[[1], [2], [3], [4]]

Solution options

Option 1: Add no_flatten=Falses in find_idx()

Comment: might be confusing as there can be one-to-many mapping

Option 2: Sanitize impacted code (shown below):

andes/andes/system.py

Lines 1768 to 1770 in 5ab784b

for model_idx, dest_idx in zip(model.idx.v, idxp.v):
if dest_idx not in dest.uid:
continue

Comment: IdxParam.v can still be list of lists that breaks the existing users' expectation

Option 3: Add a sanitize step to IdxParam.v using @v.setter

Comment: Seems to be a good choice to balance the complexity and consistency

@cuihantao
Copy link
Collaborator

cuihantao commented Oct 1, 2024 via email

@jinningwang
Copy link
Member Author

The trouble with github action seems come from the action setup-miniconda@v3 itself:

  /home/runner/miniconda3/condabin/conda config --add channels defaults
  Warning: Error while loading conda entry point: conda-libmamba-solver (module 'libmambapy' has no attribute 'QueryFormat')

Leave it here for now.

This issue seems to have been fixed, conda-incubator/setup-miniconda#366

Additionally, local tests show it works when using mamba-version: "<2.0.0"

@jinningwang
Copy link
Member Author

@cuihantao Finally this issue is fixed, I think it's ready to merge

@cuihantao cuihantao merged commit d534ced into CURENT:develop Oct 4, 2024
3 checks passed
@cuihantao
Copy link
Collaborator

Thanks Jinning! Just merged it.

@jinningwang jinningwang deleted the findidx branch October 5, 2024 18:10
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.

2 participants