Adding classes for external SSE datasets by leah-mungai · Pull Request #3 · NRC-Mila/OBELiX

leah-mungai · 2025-03-26T19:50:07Z

This pull request introduces the multipl new classes. The class will include methods that remove entries already present in OBELiX to prevent duplicate records.

New classes:

LiIon
Laskowski
ShonAndMin

To do:

Add filters to ShonandMin using @jamalabouhaibeh's script
For Laskowski and LiIon, make a method called remove_OBELiX. Using the tow columns: Laskowski ID and LiIon ID
Methods to combine all datasets composition only without duplicates: implement the merge method from pandas in Dataset
Methods to combine all datasets composition + spg without duplicates
Add test for LiIon in tests/. Test them using pytest
Add test for Laskowski
Add test for Shon&Min
Add tests for different methods (addition)

For the last two points it is a good idea o make a script that creates a datatset of all compositions and ICs at RT from all 4 dataset. This will be a good guide for what we need and will be a good tutorial later.

ftherrien · 2025-03-26T23:35:33Z

@leah-mungai Can you add a short description of what the PR does?

ftherrien

Great job overall thanks!

leah-mungai · 2025-04-03T18:44:38Z

Great job overall thanks!

Thank you Felix. I have made the changes as requested. The answer as to why I was catching and re-raising the error is that, initially when reading the dataset_url I was not including the raw file content format at the start of my url and so my pandas was giving me errors, but I corrected that.

…rrors. Updated data paths and how data is downloaded. Changes made to both class LiIon and Laskowski.

ftherrien · 2025-04-22T13:17:28Z

+        dataframe (pd.DataFrame): DataFrame containing the dataset.
+    '''
+
+    def __init__(self, data_path="/home/leah/leah---OBELiX/data/Shon_and_Min.xlsx"):


The datapath here should be something like rawdata_SM

ftherrien · 2025-04-22T13:26:44Z

+        pd.DataFrame: Filtered DataFrame.
+    """
+    filtered_df = df[(df[temp_col] >= room_temp - tolerance) & (df[temp_col] <= room_temp + tolerance)]
+    return filtered_df.drop_duplicates(subset=subset)


I don't think I would put drop duplicates here because it does not really relate to "room temperature only"

…Shon and Min dataset

…hods: remove_obelix, and get_unique_compositions.

ftherrien

Could you download it directly from the SI of the paper, I would prefer not to host our own version:

https://pubs.acs.org/doi/suppl/10.1021/acsomega.3c01424/suppl_file/ao3c01424_si_001.xlsx

ftherrien · 2025-04-29T12:59:55Z

+            to_drop.append(idx)
+            continue
+
+    df_clean = df.drop(index=to_drop).reset_index(drop=True)


I would keep the original index here

ftherrien · 2025-04-29T13:02:11Z

+    df_clean = df.drop(index=to_drop).reset_index(drop=True)
+
+    # 5) Compute log10 target
+    df_clean["log_target"] = np.log10(df_clean["Ionic Conductivity Numeric (S/cm)"])


I would not apply log10 here because we don't apply it for other dataset, we can let users decide to do that

ftherrien · 2025-04-29T13:08:55Z

+        return False
+
+# Main processing pipeline
+def main(input_xlsx: str = "ao3c01424_si_001.xlsx",


Maybe rename this function to something that explains what it does, like `clean_shon_min

ftherrien · 2025-04-29T13:10:52Z

+   # def read_data(self, data_path, no_cifs=False):
+   #     '''Reads the LiIon dataset.'''
+   #     data = pd.read_csv(self.data_path / "LiIonDatabase.csv", index_col="ID")
+
+   #     return data
+


You can remove this

ftherrien · 2025-04-29T13:11:35Z

@@ -231,11 +234,27 @@ def download_data(self, output_path, commit_id=None, local=False):
        df.to_csv(output_path / "LiIonDatabase.csv")

    def read_data(self, data_path, no_cifs=False):


Add an argument to choose to remove room temperatue only and make it true by default

ftherrien · 2025-04-29T13:12:59Z

+    def __init__(self, data_path="./SM_rawdata"):
        '''
-        Loads the Shon and Min dataset from an Excel file.
+        Loads and cleans the Shon and Min dataset from Sheet2.


"Loads and cleans the Shon and Min dataset from [the url of the excel sheet]"

ftherrien · 2025-04-29T13:13:48Z

-        data = pd.read_excel(data_path)
+        self.data_path.mkdir(parents=True, exist_ok=True)
+
+        self.source_file = Path("/home/leah/leah---OBELiX/data/ao3c01424_si_001.xlsx")


Replace this by the url. This would only work on your computer anyway since it is an absolute path

Replace this by a URL to our repo

Make it a link to your Github for now and we will change it once we merge into main here

ftherrien · 2025-04-29T13:16:08Z

+    def _clean_data(self, df):
+        '''Processes and cleans the raw dataframe.'''
+
+        df["Ionic Conductivity"] = df["Ionic Conductivity"].apply(normalize_string)
+
+        df["Ionic Conductivity Numeric"] = df["Ionic Conductivity"].apply(convert_scientific_string)
+
+        df["Ionic Conductivity Numeric (S/cm)"] = df.apply(
+            lambda r: convert_to_S_cm(r.get("Raw_unit", ""), r["Ionic Conductivity Numeric"]),
+            axis=1
+        )
+
+        to_drop = []
+        for idx, row in df.iterrows():
+            cond = row["Ionic Conductivity Numeric (S/cm)"]
+            if pd.isna(cond) or cond <= 0 or not (-18 <= np.log10(cond) <= 0):
+                to_drop.append(idx)
+                continue
+            name = row.get("Name", "")
+            if pd.isna(name) or not is_valid_formula(name):
+                to_drop.append(idx)
+                continue
+
+        df_clean = df.drop(index=to_drop).reset_index(drop=True)
+
+        df_clean["log_target"] = np.log10(df_clean["Ionic Conductivity Numeric (S/cm)"])
+
+        return df_clean


This is the same as main in shon_min.py. You can just rename main there and import directly, you don't need to copy it here

ftherrien · 2025-04-29T13:18:16Z

    '''

-    def __init__(self, data_path="/home/leah/leah---OBELiX/data/Shon_and_Min.xlsx"):
+    def __init__(self, data_path="./SM_rawdata"):


I would add an argument to choose to clean the data (e.g. clean_data = True) and set it to true by default so that if someone wants the raw dataset they can have it.

ftherrien · 2025-04-29T13:32:02Z

+    filtered_obelix_liion = ob.dataframe[ob.dataframe['Liion ID'].isin(obelix_liion_ids)]
+    reduced_compositions_liion = filtered_obelix_liion['Reduced Composition'].dropna().unique()
+
+    filtered_liion = li.dataframe[~li.dataframe['composition'].isin(reduced_compositions_liion)]


You don't need to use composition because you already have the ids in obelix_liion_ids. filtered_liion = li.dataframe.loc[~obelix_liion_ids] or something like that

The LiIon dataset doesn’t contain a Liion ID column, so although I can identify relevant Liion IDs in the Obelix dataset, I can’t directly use them to filter entries in LiIon. That’s why I opted to use the composition column in LiIon and match it with the reduced composition

ftherrien · 2025-04-29T13:42:07Z

+
+        return df_clean
+
+def remove_obelix(OBELiX, LiIon, Laskowski):


Make this a method of LiIon and Laskowski:

def remove_obelix(self, obelix_object=Obelix())

Usage:

la = Laskowski() la_no_obelix = la.remove_obelix()

ftherrien · 2025-04-29T13:58:33Z

+
+    return filtered_liion, filtered_laskowski
+
+def get_unique_compositions(OBELiX, LiIon, Laskowski, ShonAndMin):


Would be cool to implement _add_() methods. It would make most sense as a method of the Dataset class

def _add_(self, dataset): if "Space group #" in self.dataframe and "Space group #" in dataset.dataframe: return Dataset(pd.concatenate([self.dataframe.loc[["Composition","Space group #", "Ionic Conductivity"]], dataset.loc[["Composition","Space group #", "Ionic Conductivity"]])) else: return Dataset(pd.concatenate([self.dataframe.loc[["Composition", "Ionic Conductivity"]], dataset.loc[["Composition", "Ionic Conductivity"]]))

Usage:

data = ob + la + li

data is a dataframe with unique sets of composition and IC.

You could also have union where union would remove duplicates.

Since each dataset uses different column names for composition and ionic conductivity, would it be better to rename the relevant columns within each dataset class during initialization, or should I handle all the renaming centrally within the add method before combining the datasets?

ftherrien · 2025-04-29T13:59:09Z

+
+    all_compositions = pd.concat([ob_compositions, li_compositions, la_compositions, sm_compositions]).drop_duplicates()
+
+    return all_compositions.sort_values().reset_index(drop=True)


Don't forget to keep ionic conductivity

…Laskowski to automatically remove obelix entries

… ShonAndMin datasets

…plicate as an attribute

… add_DOIs method to Laskowski class

ftherrien · 2025-05-27T14:09:31Z

+
+         liion_ids = liion_ids[liion_ids < len(self.dataframe)]
+
+         rows_to_drop = self.dataframe.iloc[liion_ids]
+         return self.dataframe.drop(index=rows_to_drop.index)


I think here you only need:
return self.dataframe.drop[liion_ids]

…ula. Added a new method to laskowski to find entries where the compositions matches between obelix and laskowski but atleast one of the DOI is missing

…ethods for Laskowski

…ies. Updated test_ShonAndMin to keep minimum conductivity for duplicated entries.

…Updated class ShonAndMin to keep minimum conductivity for duplicated entries.

…dataset class, and shonandmin.py with ShonAndMin dataset class.

…e other test methods.

ftherrien · 2025-07-02T14:23:38Z

+                        for j, test_row in self.dataframe.iterrows():
+                            test_doi = test_row.get('DOI')
+                            if is_same_formula(test_row['Reduced Composition'], self_comp) and test_doi == self_doi:
+                                indices_to_remove.add(j)
+                        break


You could replace:

for j, test_row in self.dataframe.iterrows(): test_doi = test_row.get('DOI') if is_same_formula(test_row['Reduced Composition'], self_comp) and test_doi == self_doi: indices_to_remove.add(j) break

with only

indices_to_remove.add(i)

If you want to find entries with same composition where there is no DOI then I think you need:

for j, test_row in self.dataframe.iterrows(): test_doi = test_row.get('DOI') if is_same_formula(test_row['Reduced Composition'], self_comp) and test_doi is None: indices_to_remove.add(j) break

ftherrien · 2025-07-02T14:29:30Z

+        self.dataframe = self.dataframe.drop(index=indices_to_remove)
+        return self.dataframe


Return a new Dataset object here:

new_dataframe = self.dataframe.drop(index=indices_to_remove) return Dataset(new_dataframe)

ftherrien · 2025-07-02T14:35:13Z

+         ob_df = obelix_object.dataframe
+         liion_ids = ob_df["Liion ID"].dropna().astype(int)
+
+         self.dataframe.loc[self.dataframe.index.intersection(liion_ids)].to_csv('liion_obelix_matching_entries.csv', index=False)


Is there an indentation problem here?

…lix methods

… expected counts

…s and same composition. The method also keeps the original data and return the cleaned copy as a new Dataset.

- test_utils.py: 18 unit tests for is_same_formula and replace_text_IC - test_dataset.py: 46 unit tests for Dataset base class (synthetic data only) - test_datasets.py: 23 integration tests for LiIon, Laskowski, ShonAndMin - test_performance.py: 5 performance tests marked @pytest.mark.slow - conftest.py: 9 shared fixtures for edge cases and dedup testing - Register slow marker in pyproject.toml Tests target the planned API (TDD): __add__, union, remove_obelix on Laskowski/ShonAndMin will fail until implementation is complete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

added class LiIon

fbd43ff

leah-mungai added 2 commits April 1, 2025 15:15

Added class LiIon examples

c119472

Updated class LiIon methods and added class Laskowski

1ab4ad9

ftherrien requested changes Apr 1, 2025

View reviewed changes

leah-mungai added 5 commits April 8, 2025 17:52

Removed the class LiIon examples

e168588

Added two methods:is_same_formula and room_temperature_only

77c5d12

Removed the is_same_formula method and lines catching and reraising e…

1ff614a

…rrors. Updated data paths and how data is downloaded. Changes made to both class LiIon and Laskowski.

Added Shon and Min excel file

72e62a7

Added class Shon and Min

ec5c841

ftherrien requested changes Apr 22, 2025

View reviewed changes

ftherrien changed the title ~~Added class LiIon~~ Adding classes for external SSE datasets Apr 22, 2025

leah-mungai added 3 commits April 29, 2025 12:46

Added ao3c01424_si_001.xlsx which is the original Shon and Min dataset

e6384c5

Added shon_min.py script with has methods used to clean the original …

a22af68

…Shon and Min dataset

Added filters to class ShonAndMin to clean the dataset. Added two met…

662c511

…hods: remove_obelix, and get_unique_compositions.

ftherrien requested changes Apr 29, 2025

View reviewed changes

leah-mungai added 9 commits May 6, 2025 16:24

Added methods to the different classes and made changes to some methods

adea339

Renamed the main method to clean_shon_min

84a95f9

Added merge_datasets method to class Dataset. Edited class LiIon and …

d4376ba

…Laskowski to automatically remove obelix entries

Added test_dataset file to test methods used in Lilon, Laskowski, and…

a26ddff

… ShonAndMin datasets

Made minor edits

4ad0fb1

Added more assertions to the test methods in test_datasets script

159739c

Edited the merge_dataset method in class Dataset to include remove_du…

f9ad7b3

…plicate as an attribute

Edited the remove_obelix method in classes Lilon and Laskowski. Added…

c4e8491

… add_DOIs method to Laskowski class

Added preprocessing file to obelix folder

8550201

ftherrien reviewed May 27, 2025

View reviewed changes

Added laskowski_with_dois.csv, Laskowski database with DOIs

07ee89d

leah-mungai added 13 commits June 3, 2025 15:34

Made changes to the remove_overlines method.

d99e1c0

Removed the add_DOI method. Edited the remove_obelix methods

61f04f9

Removed preprocessing.py from obelix

83cfaca

Edited the laskowski remove_obelix method to include the is_same_form…

89587d6

…ula. Added a new method to laskowski to find entries where the compositions matches between obelix and laskowski but atleast one of the DOI is missing

Updated test_LiIon and test_Laskowski methods

e8b8c0b

Updated the test_Laskowski method

390f634

Edited remove_obelix and print_composition_matches_with_missing_doi m…

e5058b9

…ethods for Laskowski

Updated test_LiIon and test_Laskowski to include remove_matching_entr…

c1413df

…ies. Updated test_ShonAndMin to keep minimum conductivity for duplicated entries.

Added remove_matching_entries method to classes LiIon and Laskowski. …

5f5f5db

…Updated class ShonAndMin to keep minimum conductivity for duplicated entries.

Added lilon.py with LiIon dataset class, laskowski.py with Laskowski …

05d2836

…dataset class, and shonandmin.py with ShonAndMin dataset class.

Added a new test method called test_remove_matching_entries.Edited th…

e970fdd

…e other test methods.

Removed other classes apart from OBELiX class.

634696a

Added dataset.py with Dataset class

76e992c

ftherrien reviewed Jul 2, 2025

View reviewed changes

leah-mungai added 3 commits July 14, 2025 14:00

fixed indentation and removed blank lines in read_data and remove_obe…

b99998a

…lix methods

Removed dataFrame copies in remove_matching_entries tests and updated…

7203da9

… expected counts

Edited remove_matching_entries method to remove rows with missing DOI…

c2204e5

…s and same composition. The method also keeps the original data and return the cleaned copy as a new Dataset.

ftherrien assigned ftherrien and unassigned ftherrien Feb 24, 2026

josephdviviano self-assigned this Feb 24, 2026

		@@ -231,11 +234,27 @@ def download_data(self, output_path, commit_id=None, local=False):
		df.to_csv(output_path / "LiIonDatabase.csv")

		def read_data(self, data_path, no_cifs=False):


		return filtered_liion, filtered_laskowski

		def get_unique_compositions(OBELiX, LiIon, Laskowski, ShonAndMin):


		all_compositions = pd.concat([ob_compositions, li_compositions, la_compositions, sm_compositions]).drop_duplicates()

		return all_compositions.sort_values().reset_index(drop=True)

		self.dataframe = self.dataframe.drop(index=indices_to_remove)
		return self.dataframe

Uh oh!

Conversation

leah-mungai commented Mar 26, 2025 • edited by ftherrien Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ftherrien commented Mar 26, 2025

Uh oh!

ftherrien left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leah-mungai commented Apr 3, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ftherrien 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!

ftherrien Apr 29, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

leah-mungai commented Mar 26, 2025 •

edited by ftherrien

Loading

ftherrien Apr 29, 2025 •

edited

Loading